This repository has been archived by the owner on Aug 2, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rodeos with Streaming Plugin #9029
Rodeos with Streaming Plugin #9029
Changes from all commits
abf326e
b011a7b
0e1c4d2
d95ce2b
2a9b1c6
5a534f3
9cb67d8
31ad4b3
bdca9e6
5d58eb8
875cf30
08ba284
0dce5e2
baed64e
8856ad3
864fc77
fd6c082
2654c18
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be better as:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do that, you really ought to add
CONFIGURE_DEPENDS
. Even then it's not recommended by cmakeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We glob header files in other CMakeLists.txt and add that in. Is that the preferred method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is a cmake 3.12 feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some documentation to the PR description that includes these options. Some example uses would be good also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indicate the
routing_keys
areeosio::name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plugin really should have its own io_service and not use the main application thread io_service. This is fine for now. I'll create a JIRA issue to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm looking at amqp-cpp correctly you should not throw from inside
onError
. I don't think amqp-cpp is setup to handle that correctly. Instead justelog
the error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently needed to shutdown amqp-cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm looking at amqp-cpp correctly you should not throw from inside onError. I don't think amqp-cpp is setup to handle that correctly. Instead just elog the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently needed to shutdown amqp-cpp