kaisun2000 commented on a change in pull request #1487:
URL: https://github.com/apache/helix/pull/1487#discussion_r516200103
##########
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());
+ if (!ERROR_LOG_TO_ZK_ENABLED) {
Review comment:
So let us keep it this way. The reasoning is that source of truth
logging is in log file.
##########
File path: helix-common/src/main/java/org/apache/helix/SystemPropertyKeys.java
##########
@@ -82,4 +82,6 @@
// System Property Metadata Store Directory Server endpoint key
public static final String MSDS_SERVER_ENDPOINT_KEY =
MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY;
+
+ public static final String STATEUPDATEUTIL_ERROR_LOG_ENABLED =
"helix.StateUpdateUtil.errorLog.enabled";
Review comment:
@lei-xia, so let us change the name to "
STATEUPDATEUTIL_ERROR_LOG2ZK_ENABLED?
##########
File path: helix-common/src/main/java/org/apache/helix/SystemPropertyKeys.java
##########
@@ -82,4 +82,6 @@
// System Property Metadata Store Directory Server endpoint key
public static final String MSDS_SERVER_ENDPOINT_KEY =
MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY;
+
+ public static final String STATEUPDATEUTIL_ERROR_LOG_ENABLED =
"helix.StateUpdateUtil.errorLog.enabled";
Review comment:
@dasahcc, ZkSystemPropertyKeys is for Zookeeper related. This is more to
me like logging related.
##########
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:
removed.
##########
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:
removed.
##########
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:
changed.
##########
File path:
helix-core/src/test/java/org/apache/helix/util/TestStatusUpdateUtil.java
##########
@@ -0,0 +1,76 @@
+package org.apache.helix.util;
+
Review comment:
added.
##########
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);
+ }
+
+ for (int i = 0; i < n; i++) {
+ participants[i].syncStop();
Review comment:
fixed in tearing down in afterClass()
##########
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:
```
public void testSessionExpiryInTransition() throws Exception {
String errPath = PropertyPathBuilder.instanceError(clusterName,
"localhost_12918", oldSessionId,
"TestDB0", "TestDB0_0");
ZNRecord error = _gZkClient.readData(errPath);
Assert.assertNotNull(error,
"InterruptedException should happen in old session since task is
being cancelled during handleNewSession");
```
To make sure the following test would work.
##########
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:
ERROR_LOG_TO_ZK_ENABLED in StatusUpdateUtil is static. I changed to use
reflection to control the value now. So there is no need to set or clear it in
`TestStatusUpdateUtil` anymore.
##########
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 {
Review comment:
both added now.
##########
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());
+ if (!ERROR_LOG_TO_ZK_ENABLED) {
Review comment:
Good point. The log are in `publishStatusUpdateRecord' and
`publishErrorRecord`.
Here publishStatusUpdateRecord would never log to Zk anymore. The
participant side, it has trace level log to log4j, but not controller side. Let
me revise it to the way that both controller and participant side has log to
log4j then.
##########
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());
+ if (!ERROR_LOG_TO_ZK_ENABLED) {
Review comment:
Good point. The log are in `publishStatusUpdateRecord` and
`publishErrorRecord`.
Here publishStatusUpdateRecord would never log to Zk anymore. The
participant side, it has trace level log to log4j, but not controller side. Let
me revise it to the way that both controller and participant side has log to
log4j then.
----------------------------------------------------------------
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]
