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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
