[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7299: GEODE-9980: Improve error handling around GlobalSerialFilter

[GitBox <gi...@apache.org>]

dschneider-pivotal commented on a change in pull request #7299:
URL: https://github.com/apache/geode/pull/7299#discussion_r791947439



##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfiguration.java
##########
@@ -93,32 +107,45 @@ public boolean configure() {
       globalSerialFilter.setFilter();
 
       // log statement that filter is now configured
-      logger.accept("Global serial filter is now configured.");
+      infoLogger.accept("Global serial filter is now configured.");
       return true;
 
     } catch (UnsupportedOperationException e) {
-      if (hasRootCauseWithMessage(e, IllegalStateException.class,
-          "Serial filter can only be set once")) {
-
-        // log statement that filter was already configured
-        logger.accept("Global serial filter is already configured.");
-      }
+      handleUnsupportedOperationException(e);
       return false;
     }
   }
 
-  private static boolean hasRootCauseWithMessage(Throwable throwable,
+  private void handleUnsupportedOperationException(UnsupportedOperationException e) {
+    if (hasRootCauseWithMessageContaining(e, IllegalStateException.class,
+        "Serial filter can only be set once")) {
+
+      // log statement that filter was already configured
+      warnLogger.accept("Global serial filter is already configured.");
+    }
+    if (hasRootCauseWithMessageContaining(e, ClassNotFoundException.class,
+        "ObjectInputFilter")) {
+
+      // log statement that a global serial filter cannot be configured
+      errorLogger.accept(
+          "Geode was unable to configure a global serialization filter because ObjectInputFilter not found.");

Review comment:
       Is this a security issue? Should the security logger be used? I don't actually know when it should be used.
   Will user's understand that this error is expected on older jdks and can be fixed by using a newer one? It seems to me like this msg should contain some more info.

##########
File path: geode-serialization/src/main/java/org/apache/geode/internal/serialization/filter/GlobalSerialFilterConfiguration.java
##########
@@ -93,32 +107,45 @@ public boolean configure() {
       globalSerialFilter.setFilter();
 
       // log statement that filter is now configured
-      logger.accept("Global serial filter is now configured.");
+      infoLogger.accept("Global serial filter is now configured.");
       return true;
 
     } catch (UnsupportedOperationException e) {
-      if (hasRootCauseWithMessage(e, IllegalStateException.class,
-          "Serial filter can only be set once")) {
-
-        // log statement that filter was already configured
-        logger.accept("Global serial filter is already configured.");
-      }
+      handleUnsupportedOperationException(e);
       return false;
     }
   }
 
-  private static boolean hasRootCauseWithMessage(Throwable throwable,
+  private void handleUnsupportedOperationException(UnsupportedOperationException e) {
+    if (hasRootCauseWithMessageContaining(e, IllegalStateException.class,
+        "Serial filter can only be set once")) {
+
+      // log statement that filter was already configured
+      warnLogger.accept("Global serial filter is already configured.");

Review comment:
       Why is this a warning instead of info?
   If a user sees this warning what action should they take to figure out if something is wrong? 
   Here we say "serial filter". In the next msg it is "serialization filter". It seems like these should be consistent.




-- 
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: notifications-unsubscribe@geode.apache.org

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