add warn logs about invalid property versions #958
Conversation
bohhyang
requested review from
nizarm,
ZoabKapoor,
shivamgupta1,
lauraliu61b,
brycezhongqing and
PapaCharlie
| maxVersion = Long.max(maxVersion, property.getVersion()); | ||
| propertyVersion = property.getVersion(); | ||
| maxVersion = Long.max(maxVersion, propertyVersion); | ||
| if (maxVersion == -1) |
There was a problem hiding this comment.
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.
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.
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.
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.
yeah, makes sense, just fixed!
| if (maxVersion == -1) | ||
| { | ||
| _log.warn("Merged Uri properties has invalid version -1. It should be > -1."); | ||
| } |
There was a problem hiding this comment.
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.
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