[GitHub] [helix] pkuwm commented on a change in pull request #1487: Add a configuration option to allow enable/disable writing error log to ZK

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

pkuwm commented on a change in pull request #1487:
URL: https://github.com/apache/helix/pull/1487#discussion_r513892156



##########
File path: helix-core/src/main/java/org/apache/helix/util/HelixUtil.java
##########
@@ -62,6 +62,7 @@
 import org.apache.helix.model.ResourceAssignment;
 import org.apache.helix.model.ResourceConfig;
 import org.apache.helix.model.StateModelDefinition;
+import org.apache.helix.tools.ClusterVerifiers.StrictMatchExternalViewVerifier;

Review comment:
       This import is not used.

##########
File path: helix-core/src/main/java/org/apache/helix/util/HelixUtil.java
##########
@@ -470,11 +471,26 @@ public static int getSystemPropertyAsInt(String propertyKey, int propertyDefault
   }
 
   /**
-   * Get the value of system property
+   * Get the boolean value of system property
    * @param propertyKey
    * @param propertyDefaultValue
    * @return
    */
+  public static boolean getSystemPropertyAsBoolean(String propertyKey, String propertyDefaultValue) {

Review comment:
       We don't have to create this method to be redundant.
   Simply, `Boolean.getBoolean(propertyKey, propertyDefaultValue)` is already what is needed, right?

##########
File path: helix-core/src/main/java/org/apache/helix/util/StatusUpdateUtil.java
##########
@@ -560,6 +565,10 @@ void publishStatusUpdateRecord(ZNRecord record, Message message, Level level,
    */
   void publishErrorRecord(ZNRecord record, String instanceName, String updateSubPath,
       String updateKey, String sessionId, HelixDataAccessor accessor, boolean isController) {
+    _logger.error("StatusUpdate Error record: {}", record.toString());

Review comment:
       Nit, `record.toString()` is redundant, `record` is enough as `toString()` would be auto called.

##########
File path: helix-core/src/test/java/org/apache/helix/integration/manager/TestParticipantManager.java
##########
@@ -66,6 +66,9 @@
 public class TestParticipantManager extends ZkTestBase {
   private MBeanServer _server = ManagementFactory.getPlatformMBeanServer();
   private String clusterName = TestHelper.getTestClassName();
+  static {
+    System.setProperty(SystemPropertyKeys.STATEUPDATEUTIL_ERROR_LOG_ENABLED, "true");

Review comment:
       Can you help me understand why you set the property here, but clear it in `TestStatusUpdateUtil`? Why do we not put both in the test method? Otherwise, the system config would affect other tests, right?

##########
File path: helix-core/src/test/java/org/apache/helix/util/TestStatusUpdateUtil.java
##########
@@ -0,0 +1,76 @@
+package org.apache.helix.util;
+

Review comment:
       Missing apache license. Maybe you could setup apache license in your intellij?

##########
File path: helix-core/src/main/java/org/apache/helix/util/StatusUpdateUtil.java
##########
@@ -55,6 +56,10 @@
 public class StatusUpdateUtil {
   static Logger _logger = LoggerFactory.getLogger(StatusUpdateUtil.class);
 
+  public static final boolean ERROR_LOG_TO_ZK_ENABLED =
+      HelixUtil.getSystemPropertyAsBoolean(SystemPropertyKeys.STATEUPDATEUTIL_ERROR_LOG_ENABLED, "false");

Review comment:
       `Boolean.getBoolean(propertyKey, propertyDefaultValue)` is simple and good enough?

##########
File path: helix-core/src/test/java/org/apache/helix/util/TestStatusUpdateUtil.java
##########
@@ -0,0 +1,76 @@
+package org.apache.helix.util;
+
+import org.apache.helix.HelixConstants;
+import org.apache.helix.PropertyPathBuilder;
+import org.apache.helix.SystemPropertyKeys;
+import org.apache.helix.TestHelper;
+import org.apache.helix.common.ZkTestBase;
+import org.apache.helix.integration.manager.MockParticipantManager;
+import org.apache.helix.messaging.handling.HelixStateTransitionHandler;
+import org.apache.helix.model.Message;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.helix.zookeeper.zkclient.exception.ZkException;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+
+public class TestStatusUpdateUtil extends ZkTestBase {
+  private String clusterName = TestHelper.getTestClassName();
+  static {
+    System.clearProperty(SystemPropertyKeys.STATEUPDATEUTIL_ERROR_LOG_ENABLED);
+  }
+
+  @Test
+  public void testDisableErrorLogByDefault() throws Exception {
+    StatusUpdateUtil _statusUpdateUtil = new StatusUpdateUtil();
+    int n = 1;
+
+    Exception e = new RuntimeException("test exception");
+
+    TestHelper.setupCluster(clusterName, ZK_ADDR, 12918, // participant port
+        "localhost", // participant name prefix
+        "TestDB", // resource name prefix
+        1, // resources
+        1, // partitions per resource
+        n, // number of nodes
+        1, // replicas
+        "MasterSlave", true);
+
+    MockParticipantManager[] participants = new MockParticipantManager[n];
+
+    for (int i = 0; i < n; i++) {
+      String instanceName = "localhost_" + (12918 + i);
+      participants[i] = new MockParticipantManager(ZK_ADDR, clusterName, instanceName);
+      participants[i].syncStart();
+    }
+
+    Message message = new Message(Message.MessageType.STATE_TRANSITION, "Some unique id");
+    message.setSrcName("cm-instance-0");
+    message.setTgtSessionId(participants[0].getSessionId());
+    message.setFromState("Offline");
+    message.setToState("Slave");
+    message.setPartitionName("TestDB_0");
+    message.setMsgId("Some unique message id");
+    message.setResourceName("TestDB");
+    message.setTgtName("localhost_12918");
+    message.setStateModelDef("MasterSlave");
+    message.setStateModelFactoryName(HelixConstants.DEFAULT_STATE_MODEL_FACTORY);
+    _statusUpdateUtil.logError(message, HelixStateTransitionHandler.class, e,
+        "test status update", participants[0]);
+
+    // assert by default, not logged to Zookeeper
+    String errPath = PropertyPathBuilder.instanceError(clusterName, "localhost_12918",
+        participants[0].getSessionId(),
+        "TestDB", "TestDB_0");
+    try {
+      ZNRecord error = _gZkClient.readData(errPath);
+      Assert.fail("not expecting being able to send error logs to ZK by default.");
+    } catch (ZkException zke) {
+      Assert.assertTrue(true);

Review comment:
       Why `assertTrue(True);` which is always true? Just put a comment it is expected. Or to be more accurate, we can assert the exception message to ensure the exception is the expected one not others(same exception type but different message thrown from other places). 




----------------------------------------------------------------
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org