Yingyi Bu has posted comments on this change. Change subject: Introduce Strategy Based Replication and Fault-Tolerance ......................................................................
Patch Set 14: (14 comments) There are quite a few new functionalities and new classes. The added metadata_node_recovery test is nice but it is sth. that should work before this change and hence can only protect regressions. Can you add 1) MockIto-based unit tests and 2) end-to-end recovery tests if possible to make sure the new code behaves as expected? Other detailed comments are inlined. https://asterix-gerrit.ics.uci.edu/#/c/1405/14/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/replication/MetadataNodeFaultToleranceStrategy.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/replication/MetadataNodeFaultToleranceStrategy.java: PS14, Line 73: synchronized why synchronized is needed for each method? The root call from RemoveDeadNodesWork is run in a single-threaded WorkQueue. PS14, Line 132: HyracksDataException Use ErrorCode: https://cwiki.apache.org/confluence/display/ASTERIXDB/Exception+Handling PS14, Line 144: log Check logging level before the log call? https://asterix-gerrit.ics.uci.edu/#/c/1405/14/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/replication/NoFaultToleranceStrategy.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/replication/NoFaultToleranceStrategy.java: PS14, Line 84: Use ErrorCode: https://cwiki.apache.org/confluence/display/ASTERIXDB/Exception+Handling https://asterix-gerrit.ics.uci.edu/#/c/1405/14/asterixdb/asterix-app/src/main/java/org/apache/asterix/util/FaultToleranceUtil.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/util/FaultToleranceUtil.java: PS14, Line 62: LOGGER Check logging level before calling log? https://asterix-gerrit.ics.uci.edu/#/c/1405/14/asterixdb/asterix-app/src/test/resources/runtimets/results/api/replication/replication.1.adm File asterixdb/asterix-app/src/test/resources/runtimets/results/api/replication/replication.1.adm: PS14, Line 3: batchsize Where do the original information "enabled" and "factor" go now? https://asterix-gerrit.ics.uci.edu/#/c/1405/14/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/ClusterProperties.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/ClusterProperties.java: PS14, Line 78: getNode getNode() -> getNodes() getNode().get(i) feels confusing to me. https://asterix-gerrit.ics.uci.edu/#/c/1405/14/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/replication/ChainedDeclusteringReplicationStrategy.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/replication/ChainedDeclusteringReplicationStrategy.java: PS14, Line 48: log Check logging level before the log call? PS14, Line 79: IllegalStateException Using error code? https://cwiki.apache.org/confluence/display/ASTERIXDB/Exception+Handling https://asterix-gerrit.ics.uci.edu/#/c/1405/14/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/replication/ReplicationStrategyFactory.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/replication/ReplicationStrategyFactory.java: PS14, Line 52: IllegalArgumentException Use error code? https://cwiki.apache.org/confluence/display/ASTERIXDB/Exception+Handling PS14, Line 59: IllegalStateException Error code? https://asterix-gerrit.ics.uci.edu/#/c/1405/14/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/IRecoveryManager.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/IRecoveryManager.java: PS14, Line 37: INITIAL_RUN comment what's the difference between INITIAL_RUN and NEW_UNIVERSE? I couldn't tell the difference only based on the words. https://asterix-gerrit.ics.uci.edu/#/c/1405/14/asterixdb/asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java File asterixdb/asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java: PS14, Line 1123: rm Use File API to remove? We hope all regression tests can work on Windows platforms. https://asterix-gerrit.ics.uci.edu/#/c/1405/14/asterixdb/asterix-installer/src/test/java/org/apache/asterix/installer/test/MetadataReplicationIT.java File asterixdb/asterix-installer/src/test/java/org/apache/asterix/installer/test/MetadataReplicationIT.java: PS14, Line 44: integrationts Use File separator for Windows compatibility. -- To view, visit https://asterix-gerrit.ics.uci.edu/1405 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d1012f5541ce786f127866efefb9f3db434fedd Gerrit-PatchSet: 14 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
