dombizita commented on code in PR #5490:
URL: https://github.com/apache/ozone/pull/5490#discussion_r1407511837
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/tasks/TestOMDBUpdatesHandler.java:
##########
@@ -197,18 +195,8 @@ public void testDelete() throws Exception {
OmVolumeArgs volumeInfo = (OmVolumeArgs) volEvent.getValue();
assertEquals("sampleVol", volumeInfo.getVolume());
- // Assert the values of non existent keys are set to null.
- OMDBUpdateEvent nonExistKey = events.get(2);
- assertEquals(OMDBUpdateEvent.OMDBUpdateAction.DELETE,
- nonExistKey.getAction());
- assertEquals("/sampleVol/bucketOne/key_two", nonExistKey.getKey());
- assertNull(nonExistKey.getValue());
-
- OMDBUpdateEvent nonExistVolume = events.get(3);
- assertEquals(OMDBUpdateEvent.OMDBUpdateAction.DELETE,
- nonExistVolume.getAction());
- assertEquals(nonExistVolumeKey, nonExistVolume.getKey());
- assertNull(nonExistVolume.getValue());
+ // Assert for non existent keys, no events will be captured and handled.
+ assertEquals(2, events.size());
Review Comment:
I don't think this is necessary, we are already checking this on line 184,
but the comment is good, we can move that there.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/OMDBUpdatesHandler.java:
##########
@@ -153,8 +155,22 @@ private void processEvent(int cfIndex, byte[] keyBytes,
byte[]
}
}
} else if (action.equals(DELETE)) {
- if (oldValue != null && !omUpdateEventValidator.isValidEvent(tableName,
- oldValue, key, action)) {
+ if (null == oldValue) {
+ String keyStr = (key instanceof String) ? key.toString() : "";
+ if (keyStr.isEmpty()) {
+ LOG.warn(
+ "Only DTOKEN_TABLE table uses OzoneTokenIdentifier as key " +
+ "instead of String. Event on any other table in this " +
+ "condition may need to be investigated. This DELETE " +
+ "event is on {} table which is not useful for Recon to " +
+ "capture.", tableName);
+ }
+ LOG.warn("Old Value of Key: {} in table: {} should not be null " +
+ "for DELETE event ", keyStr, tableName);
+ return;
Review Comment:
For me this is more convenient and also we are not printing the warn message
with the empty `keyStr`, which I'm not sure if it is a good idea. We won't have
any new information in that message, as the `tableName` is already logged in
the other message, but we won't have the "old value is null" information with
this approach if the key is not a String. What do you think?
```suggestion
if (oldValue == null) {
if (key instanceof String) {
LOG.warn("Old Value of Key: {} in table: {} should not be null "
+
"for DELETE event ", key.toString(), tableName);
} else {
LOG.warn(
"Only DTOKEN_TABLE table uses OzoneTokenIdentifier as key " +
"instead of String. Event on any other table in this " +
"condition may need to be investigated. This DELETE " +
"event is on {} table which is not useful for Recon to "
+
"capture.", tableName);
}
return;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
