Murtadha Hubail has posted comments on this change. Change subject: Asterix NCs Failback Support ......................................................................
Patch Set 4: (12 comments) @Abdullah, I addressed your comments and also updated the fully descriptive test files names that you criticised :) https://asterix-gerrit.ics.uci.edu/#/c/613/4/asterix-app/src/main/java/org/apache/asterix/api/common/AsterixAppRuntimeContext.java File asterix-app/src/main/java/org/apache/asterix/api/common/AsterixAppRuntimeContext.java: Line 183: PersistentLocalResourceRepository persistentLocalResourceRepository = (PersistentLocalResourceRepository) localResourceRepository; > instead of down casting, why not change localResourceRepository type. Done https://asterix-gerrit.ics.uci.edu/#/c/613/4/asterix-common/src/main/java/org/apache/asterix/common/replication/NodeFailbackPlan.java File asterix-common/src/main/java/org/apache/asterix/common/replication/NodeFailbackPlan.java: Line 31: public enum FailbackPlanState { > Can you add comments to make sure the state transition is clear? Done. I also added comments on the possible states in AsterixClusterProperties at each stage of the plan lifecycle. https://asterix-gerrit.ics.uci.edu/#/c/613/4/asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java File asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java: Line 223: System.out.println("Repinse " + errorBody); > Fix as you see fit. Done Line 265: HttpMethodBase method = new GetMethod(url);; > remove additional semi-colon Done Line 527: if (queryCount == expectedResultFileCtxs.size() - 1) { > https://asterix-gerrit.ics.uci.edu/#/c/625/12/asterix-common/src/test/java/ As we discussed, I remove this and added it to the finally block similar to your change. Line 629: "http://"; + host + ":" + port + "/admin/cluster"); > get the url from the servlet definition so if it changes, we don't need to Done https://asterix-gerrit.ics.uci.edu/#/c/613/4/asterix-om/src/main/java/org/apache/asterix/om/util/AsterixClusterProperties.java File asterix-om/src/main/java/org/apache/asterix/om/util/AsterixClusterProperties.java: Line 214: state = ClusterState.ACTIVE; > change to while not 0. get rid of recursion Done Line 584: } else { > fix bug. else if failed, then revert or something Done https://asterix-gerrit.ics.uci.edu/#/c/613/4/asterix-replication/src/main/java/org/apache/asterix/replication/management/ReplicationManager.java File asterix-replication/src/main/java/org/apache/asterix/replication/management/ReplicationManager.java: Line 277: break; > doesn't look nice. find a better way?? Done Line 300: //if the component belong to a shit > Fix comment Done https://asterix-gerrit.ics.uci.edu/#/c/613/4/asterix-replication/src/main/java/org/apache/asterix/replication/recovery/RemoteRecoveryManager.java File asterix-replication/src/main/java/org/apache/asterix/replication/recovery/RemoteRecoveryManager.java: Line 72: int maxRecoveryAttempts = 5; > get from a configuration file? I changed it to be read from AsterixReplicationProperties but currently it is just a constant, but it can be read from a config file later. Line 251: public void startFailbackProcess() { > get from conf? I changed it to be read from AsterixReplicationProperties but currently it is just a constant, but it can be read from a config file later. -- To view, visit https://asterix-gerrit.ics.uci.edu/613 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id17819542d6b9c4e32647e64737c4a467b630f24 Gerrit-PatchSet: 4 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
