[GitHub] [ozone] devmadhuu commented on a diff in pull request #5043: HDDS-8310. Recon goes down with RepeatedOmKeyInfo cannot be cast to OmKeyInfo

["devmadhuu (via GitHub)" <gi...@apache.org>]

devmadhuu commented on code in PR #5043:
URL: https://github.com/apache/ozone/pull/5043#discussion_r1260552251


##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/FileSizeCountTask.java:
##########
@@ -158,50 +158,64 @@ public Pair<String, Boolean> process(OMUpdateEventBatch events) {
       String updatedKey = omdbUpdateEvent.getKey();
       Object value = omdbUpdateEvent.getValue();
       Object oldValue = omdbUpdateEvent.getOldValue();
+      // Check if values are instance of OmKeyInfo
+      if (!isOmKeyInfo(value)) {
+        continue;
+      }
+      OmKeyInfo omKeyInfo = (OmKeyInfo) value;
 
-      if (value instanceof OmKeyInfo) {
-        OmKeyInfo omKeyInfo = (OmKeyInfo) value;
-        OmKeyInfo omKeyInfoOld = (OmKeyInfo) oldValue;
-
-        try {
-          switch (omdbUpdateEvent.getAction()) {
-          case PUT:
-            handlePutKeyEvent(omKeyInfo, fileSizeCountMap);
-            break;
-
-          case DELETE:
-            handleDeleteKeyEvent(updatedKey, omKeyInfo, fileSizeCountMap);
-            break;
+      try {
+        switch (omdbUpdateEvent.getAction()) {
+        case PUT:
+          handlePutKeyEvent(omKeyInfo, fileSizeCountMap);
+          break;
 
-          case UPDATE:
-            if (omKeyInfoOld != null) {
-              handleDeleteKeyEvent(updatedKey, omKeyInfoOld, fileSizeCountMap);
-              handlePutKeyEvent(omKeyInfo, fileSizeCountMap);
-            } else {
-              LOG.warn("Update event does not have the old keyInfo for {}.",
-                  updatedKey);
-            }
-            break;
+        case DELETE:
+          handleDeleteKeyEvent(updatedKey, omKeyInfo, fileSizeCountMap);
+          break;
 
-          default:
-            LOG.trace("Skipping DB update event : {}",
-                omdbUpdateEvent.getAction());
+        case UPDATE:
+          if (oldValue != null && isOmKeyInfo(oldValue)) {
+            OmKeyInfo omKeyInfoOld = (OmKeyInfo) oldValue;
+            handleDeleteKeyEvent(updatedKey, omKeyInfoOld, fileSizeCountMap);
+            handlePutKeyEvent(omKeyInfo, fileSizeCountMap);
+          } else {
+            LOG.warn("Update event does not have the old keyInfo for {}.",

Review Comment:
   Message may not be reflecting both conditions here which one didn't satisfy.



##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ContainerKeyMapperTask.java:
##########
@@ -216,6 +216,10 @@ public Pair<String, Boolean> process(OMUpdateEventBatch events) {
         continue;
       }
       String updatedKey = omdbUpdateEvent.getKey();
+      if (!isOmKeyInfo(omdbUpdateEvent.getValue())) {

Review Comment:
   you can combine this if condition with if condition at line 215 to avoid line 218 execution.



##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/NSSummaryTaskWithLegacy.java:
##########
@@ -206,6 +206,22 @@ public boolean processWithLegacy(OMUpdateEventBatch events) {
     return true;
   }
 
+  /**
+   * Checks if the object passed is an instance of `OmKeyInfo`.
+   *
+   * @param obj The object to check.
+   * @return True if the object is an instance of `OmKeyInfo`, false otherwise.
+   */
+  private boolean isOmKeyInfo(Object obj) {

Review Comment:
   Can we move this method at common place ? We may not need this here or in any class if we add a logic in org.apache.hadoop.ozone.recon.tasks.OMDBUpdatesHandler#processEvent



##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/tasks/ContainerKeyMapperTask.java:
##########
@@ -263,6 +268,22 @@ public Pair<String, Boolean> process(OMUpdateEventBatch events) {
     return new ImmutablePair<>(getTaskName(), true);
   }
 
+  /**
+   * Checks if the object passed is an instance of `OmKeyInfo`.
+   *
+   * @param obj The object to check.
+   * @return True if the object is an instance of `OmKeyInfo`, false otherwise.
+   */
+  private boolean isOmKeyInfo(Object obj) {
+    if (obj instanceof OmKeyInfo) {
+      return true;
+    } else {
+      LOG.warn("Unexpected value type {} for key. Skipping processing.",
+          obj.getClass().getName());
+      return false;

Review Comment:
   @ArafatKhan2198 thanks for providing workaround, however as we know ideally keyTable and fileTable should not have RepeatedKeyInfo objects and events for these key types should not raised for these 2 tables. So as @smengcl suggested, may be you can add some validation logic in "`org.apache.hadoop.ozone.recon.tasks.OMDBUpdatesHandler#processEvent`" where you can maintain an enum or map for keyType class name with its corresponding tablename as reference data to validate against and while if validation fails, you can log as ERROR with details like keyType, tablename, event action (PUT, DELETE, UPDATE etc), and  not allow such events to process as well to avoid flowing downstream. This way we can stop such events there itself and change will be at one place instead of all classes.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org