diff options
author | valerijf <valerijf@yahoo-inc.com> | 2017-04-04 09:36:03 +0200 |
---|---|---|
committer | valerijf <valerijf@yahoo-inc.com> | 2017-04-04 09:36:03 +0200 |
commit | abf8b7ffc81efc0bfff4429d5ae265a049591de7 (patch) | |
tree | 8b798d77ac61eb865a01aded306f1b93c5a663a2 /node-admin/src | |
parent | cc34554b38d10fb110ef9d2a4b7d2a3539fb5cee (diff) |
Code review fixes
Diffstat (limited to 'node-admin/src')
3 files changed, 16 insertions, 10 deletions
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java index fad87492f83..4748670a4be 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java @@ -74,7 +74,9 @@ public class NodeAdminImpl implements NodeAdmin { } }, 0, 30, TimeUnit.SECONDS); - aclMaintainer.ifPresent(maintainer -> aclScheduler.scheduleAtFixedRate(maintainer, 30, 60, TimeUnit.SECONDS)); + aclMaintainer.ifPresent(maintainer -> aclScheduler.scheduleAtFixedRate(() -> { + if (!isFrozen()) maintainer.run(); + }, 30, 60, TimeUnit.SECONDS)); } @Override @@ -102,11 +104,16 @@ public class NodeAdminImpl implements NodeAdmin { } @Override - public boolean setFrozen(boolean frozen) { + public boolean setFrozen(boolean wantFrozen) { + // Use filter with count instead of allMatch() because allMatch() will short curcuit on first non-match boolean allNodeAgentsConverged = nodeAgents.values().stream() - .filter(nodeAgent -> !nodeAgent.setFrozen(frozen)) + .filter(nodeAgent -> !nodeAgent.setFrozen(wantFrozen)) .count() == 0; - if (allNodeAgentsConverged) isFrozen = frozen; + + if (wantFrozen) { + if (allNodeAgentsConverged) isFrozen = true; + } else isFrozen = false; + return allNodeAgentsConverged; } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java index 201df8c5db2..d581a6fadc6 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminStateUpdater.java @@ -130,7 +130,7 @@ public class NodeAdminStateUpdater extends AbstractComponent { } /** - * This method is attempts to converge NodeAgent's and NodeAdmin's frozen state with their orchestrator + * This method attempts to converge NodeAgent's and NodeAdmin's frozen state with their orchestrator * state. When trying to suspend node-admin, this method will first attempt to freeze all NodeAgents and * NodeAdmin, then asking orchestrator for permission to suspend node-admin app, and finally asking orchestrator * for permission to suspend all active nodes on this host, if either of the request is denied, @@ -139,8 +139,7 @@ public class NodeAdminStateUpdater extends AbstractComponent { private void convergeState(State wantedState) { boolean wantFrozen = wantedState != RESUMED; if (!nodeAdmin.setFrozen(wantFrozen)) { - throw new RuntimeException("Wanted NodeAdmin to be " + (wantFrozen ? "frozen" : "unfrozen") + - " but instead is " + (!wantFrozen ? "frozen" : "unfrozen")); + throw new RuntimeException("NodeAdmin has not yet converged to " + (wantFrozen ? "frozen" : "unfrozen")); } // To get to resumed state, we only need to converge NodeAdmins frozen state diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java index 90a6a04c5a9..3594a16b182 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java @@ -136,7 +136,7 @@ public class NodeAdminImplTest { nodeAdmin.synchronizeNodeSpecsToNodeAgents(nodeSpecs, existingContainers); mockNodeAgentSetFrozenResponse(nodeAgents, false, false, false); - assertFalse(nodeAdmin.setFrozen(true)); + assertFalse(nodeAdmin.setFrozen(true)); // NodeAdmin freezes only when all the NodeAgents are frozen mockNodeAgentSetFrozenResponse(nodeAgents, false, true, true); assertFalse(nodeAdmin.setFrozen(true)); @@ -152,11 +152,11 @@ public class NodeAdminImplTest { mockNodeAgentSetFrozenResponse(nodeAgents, false, false, false); assertFalse(nodeAdmin.setFrozen(false)); - assertTrue(nodeAdmin.isFrozen()); + assertFalse(nodeAdmin.isFrozen()); // NodeAdmin unfreezes instantly mockNodeAgentSetFrozenResponse(nodeAgents, false, false, true); assertFalse(nodeAdmin.setFrozen(false)); - assertTrue(nodeAdmin.isFrozen()); + assertFalse(nodeAdmin.isFrozen()); mockNodeAgentSetFrozenResponse(nodeAgents, true, true, true); assertTrue(nodeAdmin.setFrozen(false)); |