-
Notifications
You must be signed in to change notification settings - Fork 547
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
add warn logs about invalid property versions #958
Conversation
maxVersion = Long.max(maxVersion, property.getVersion()); | ||
propertyVersion = property.getVersion(); | ||
maxVersion = Long.max(maxVersion, propertyVersion); | ||
if (maxVersion == -1) |
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.
Should we change the logic to
if (propertyVersion <= -1) ?
As if the propertiesToMerges
is (1, -1, -10), there is not warning.
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.
that's ok since the -1, -10 will be warned already when they were received in ZooKeeperEphemeralStore.java. Here we just warn for the max version result. But indeed, we could just log it at the end instead of every uri check. I'll move 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.
Make sense. Thanks
|
||
public class UriPropertiesMerger implements ZooKeeperPropertyMerger<UriProperties> | ||
{ | ||
private static final Logger _log = LoggerFactory.getLogger(UriPropertiesMerger.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.
I was going to comment that I thought it's convention to use LOG
for a static final
logger, but I see that the existing codebase is inconsistent 😂 So I think this is fine
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.
yeah, makes sense, just fixed!
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.
LGTM
if (maxVersion == -1) | ||
{ | ||
_log.warn("Merged Uri properties has invalid version -1. It should be > -1."); | ||
} |
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.
Should these checks be outside the UriPropertiesMerger
code and somewhere in the dual read comparison code, since this class isn't really concerned and/or responsible about such validations?
It might also result in a lot of logs where not really required.
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.
the one in ZooKeeperEphemeralStore.java logs about the invalid data and property name. Here is where the merge-version logic happens, so it's responsible for the merged final version.
The dual read monitor is not responsible for this validation, and it won't be used anymore after migrated fully to INDIS. Plus, this merged version issue only applies to uris, whereas the dual read monitor has a generic property.
I've added a warn log when receiving xds flow uri property as well. This merger will cover both zk and xds flow for the merge logic.
It might also result in a lot of logs where not really required.
Any of this warn message is important since it means some invalid zk (and future Kafka) data and need our attention to fix.
Summary
As the title. In case bad uri property version is produced for any edge cases (connection loss, etc.), we want to warn that.
Test Done
build and test