Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read serialized qos profiles out of the metadata #359

Merged
merged 2 commits into from
Apr 9, 2020

Conversation

emersonknapp
Copy link
Collaborator

Part of #125

This PR introduces the ability to read the new field offered_qos_profiles out of a bag metadata. This required making a way to pass version information to sub-structs of BagMetadata so that backwards compatibility can be maintained.

@piraka9011
Copy link
Contributor

This is pretty interesting, how does it work for future changes to the metadata? Do we just add a layer on top of decode?

@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Apr 7, 2020

You just add if blocks for data added in new versions. That's the way that I have historically seen this done for similar data format parsing.

And if you remove fields you send up making it a version range for reading

Copy link
Contributor

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR LGTM, but this versioning approach is going to explode pretty soon if we continue doing active development. I'd suggest to spend some time post-release thinking about what we should be doing (cough Protobuf cough cough).

Copy link
Contributor

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually LGTM'ing the PR

@emersonknapp emersonknapp force-pushed the emersonknapp/read-topicmetadata-v4 branch from 78189f7 to 244755c Compare April 7, 2020 21:51
Copy link
Contributor

@piraka9011 piraka9011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: Pending Thomas comments and Green CI

@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Apr 8, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp emersonknapp force-pushed the emersonknapp/read-topicmetadata-v4 branch from ef07003 to af9c04a Compare April 8, 2020 00:24
@jacobperron
Copy link
Member

jacobperron commented Apr 8, 2020

@emersonknapp FYI, we're having a windows build issue that just started happening. I cleared a build from the queue and edited your comment to point to the right build number, but I think you might hit the same unrelated build failure in resource_retriever.

Edit: related issue ros/resource_retriever#38

@emersonknapp
Copy link
Collaborator Author

Thanks for the heads up!

@emersonknapp
Copy link
Collaborator Author

emersonknapp commented Apr 9, 2020

Running CI again before merging since a few minor changes have gone into this and a lot has gone into the upstream

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

Retrying windows Build Status

Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/read-topicmetadata-v4 branch from eb8c66f to b3ba4c7 Compare April 9, 2020 17:54
@emersonknapp
Copy link
Collaborator Author

The test failures are clearly unrelated, having to do with typesupport for connext and cyclone, on windows, in the last two runs. We need to fix that, but I'm going to merge this change to unblock the followup work.

@emersonknapp emersonknapp merged commit adfb6d6 into master Apr 9, 2020
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/read-topicmetadata-v4 branch April 9, 2020 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants