-
Notifications
You must be signed in to change notification settings - Fork 283
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
VMRouter should add message tracking information #6368
base: development
Are you sure you want to change the base?
Conversation
Basic testing successful.
Test channels: |
@@ -112,3 +112,6 @@ database.connection.retrywaitinmilliseconds = 10000 | |||
# database-readonly.url = jdbc:... | |||
# | |||
database.enable-read-write-split = true | |||
|
|||
# If true, enable routing enhancements for improved message tracking | |||
routing.enable-enhancements = true |
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.
Why should this be a configurable option? What harmful behavior could happen if the routing was done for a use case that did not expect it?
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 had in mind any existing scripts that are looking for specific data that may break.
* | ||
* TrackingEnhancer | ||
*/ | ||
private class TrackingEnhancer { |
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.
Is there any value in making this a not inner class?
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.
Perhaps for testing. Can certainly do so if folks find it beneficial.
*/ | ||
private TrackingEnhancer(String channelId, Long messageId, SourceMap sourceMap) { | ||
this.channelId = channelId != null ? channelId : "NONE"; | ||
this.messageId = messageId != null ? messageId : -1L; |
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.
Why is this -1L and not zero?
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 followed the existing template logic which is a reasonable "non-value" value.
* @return a new Map | ||
*/ | ||
private Map<String, Object> enrich(Map<String, Object> messageSourceMap) { | ||
if (messageSourceMap == null) { |
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.
Leaving the condition is fine but is there any chance of messageSourceMap ever being null?
Should the @Null
or @NotNull
annotations be used?
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.
Yes, I see it can come in as null from at least one method.
I don't see other places where @Null
and @NotNull
are used that I could copy, so I defer to others on properly decorating and validating the code.
Initial attempt to enhance the VMRouter using @tonygermano's existing code template.
Has not yet been tested locally.