-
Notifications
You must be signed in to change notification settings - Fork 121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove node #1053
Remove node #1053
Conversation
@Maithem Could you add the checklist back to the PR comment? Thanks. |
@no2chem ok, added them along with an estimate. |
currentLayout.setRuntime(runtime); | ||
sealEpoch(currentLayout); | ||
|
||
LayoutBuilder builder = new LayoutBuilder(currentLayout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can build the layout before sealing.
So the order can be :
- generate new layout
- seal
- consensus
- reconfigure Sequencer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way if there is an error while building the new layout, you would not have sealed and blocked the entire system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
@@ -74,6 +80,18 @@ void handleQuery(CorfuPayloadMsg<OrchestratorRequest> msg, ChannelHandlerContext | |||
.payloadMsg(new OrchestratorResponse(resp ))); | |||
} | |||
|
|||
void removeNode(CorfuPayloadMsg<OrchestratorRequest> msg, ChannelHandlerContext ctx, IServerRouter r) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be combined in dispatch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is now.
Results automatically generated by CorfuDB Benchmark Framework to assess the performance of this pull request for commit 1378e8a. *** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 1 threads, Disk mode An interactive dashboard with Pull Request Performance Metrics for ALL cluster types and numbers of threads in run, is available at: |
41739cd
to
3b93c48
Compare
Results automatically generated by CorfuDB Benchmark Framework to assess the performance of this pull request for commit 3b93c48. *** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 1 threads, Disk mode An interactive dashboard with Pull Request Performance Metrics for ALL cluster types and numbers of threads in run, is available at: |
Results automatically generated by CorfuDB Benchmark Framework to assess the performance of this pull request for commit 48fe631. *** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 1 threads, Disk mode An interactive dashboard with Pull Request Performance Metrics for ALL cluster types and numbers of threads in run, is available at: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments.
I think this needs to be broken up into two PRs.
The first PR is a change to the orchestrator service. The second PR is to add remove node functionality.
r.sendResponse(ctx, msg, CorfuMsgType.ORCHESTRATOR_RESPONSE | ||
.payloadMsg(resp)); | ||
run(workflow); | ||
CompletableFuture.runAsync(() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use a CompletableFuture when you never join on it?
@@ -42,7 +39,7 @@ | |||
public class Orchestrator { | |||
|
|||
final Callable<CorfuRuntime> getRuntime; | |||
final Map<String, UUID> activeWorkflows = new ConcurrentHashMap(); | |||
final Map<UUID, String> activeWorkflows = new ConcurrentHashMap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that you need a BiMap instead...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
activeWorkflows.remove(workflow.getId()); | ||
} | ||
} | ||
|
||
UUID findUUID(String endpoint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See BiMap comment...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
if (payload.getType().equals(Types.OrchestratorRequestType.ADD_NODE)) { | ||
return new AddNodeWorkflow(payload); | ||
switch (payload.getType()) { | ||
case ADD_NODE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you just make this part of the OrchestratorRequestType enum?
Add a new method:
OrchestratorRequestType.getWorkflow(OrchestratorRequest request)
Then no need to manage this extra switch statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* @return true if all actions completed successfully | ||
*/ | ||
public boolean completed() { | ||
for (Action action : getActions()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or getActions().stream().anyMatch(a -> a.equals(ActionStatus.COMPLETED)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
// Remove a node from a two node cluster. Verify that a remove operation won't | ||
// cause the loss of redundancy | ||
assertThatThrownBy(() -> r.getLayoutManagementView().removeNode(l2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to do the second part (Verify that a remove operation won't
-
// cause the loss of redundancy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the comment
format/proto/types.proto
Outdated
} | ||
|
||
// Orchestrator responses | ||
enum OrchestratorResponseType { | ||
// The status of a workflow | ||
WORKFLOW_STATUS = 0; | ||
// Id of a created workflow | ||
WORKFLOW_ID = 1; | ||
WORKFLOW_CREATE = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is workflow_create? The comments don't match...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the name.
c36fe11
to
aebbf0a
Compare
Results automatically generated by CorfuDB Benchmark Framework to assess the performance of this pull request for commit aebbf0a. *** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 1 threads, Disk mode An interactive dashboard with Pull Request Performance Metrics for ALL cluster types and numbers of threads in run, is available at: |
LayoutModificationException, OutrankedException { | ||
|
||
currentLayout.setRuntime(runtime); | ||
sealEpoch(currentLayout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move seal after the layout creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes i think you can. We should not seal if we are going to throw an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -75,6 +81,30 @@ void handleQuery(CorfuPayloadMsg<OrchestratorRequest> msg, ChannelHandlerContext | |||
.payloadMsg(new OrchestratorResponse(resp))); | |||
} | |||
|
|||
void removeNode(CorfuPayloadMsg<OrchestratorRequest> msg, ChannelHandlerContext ctx, IServerRouter r) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed, now we only have dispatched.
RemoveNodeRequest req = (RemoveNodeRequest) msg.getPayload().getRequest(); | ||
|
||
if (findUUID(req.getEndpoint()) != null) { | ||
// An add node workflow is already executing for this endpoint, return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment out of place or is the findUUID getting add node workflows in remove node ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this looks like similar code to what you do in addNode. Can we consolidate and have less code ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored in the HOG branch that depends on this, but I'll push the change here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed, now we only have dispatch. Its refactored.
/** | ||
* @author Maithem | ||
*/ | ||
public class RemoveNodeWorkflow extends Workflow { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A short file level comment would do for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
return; | ||
} else { | ||
// Create a new workflow for this endpoint and return a new workflow id | ||
Workflow workflow = getWorkflow(msg.getPayload()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this best effort or is the driver single threaded? Because this is not protected against multiple threads?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make it synchronized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in dispatch method.
OrchestratorResponse resp = new OrchestratorResponse(new CreateWorkflowResponse(workflow.getId())); | ||
r.sendResponse(ctx, msg, CorfuMsgType.ORCHESTRATOR_RESPONSE | ||
.payloadMsg(resp)); | ||
CompletableFuture.runAsync(() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should first start the asyncWorkflow before sending response. Just for sanity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
+ "No replicas available."); | ||
} else { | ||
if (layoutSegment.getReplicationMode() == Layout.ReplicationMode.CHAIN_REPLICATION) { | ||
if (layoutStripe.getLogServers().size() < 2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should 2 go into some constant or properties file. Although 2 is redundancy so I am not sure !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a proper way to specify server properties and this seems like one of those properties.
I'll make it a constant for now.
LayoutModificationException, OutrankedException { | ||
|
||
currentLayout.setRuntime(runtime); | ||
sealEpoch(currentLayout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes i think you can. We should not seal if we are going to throw an exception.
@@ -38,7 +38,7 @@ String getConnectionString(int port) { | |||
} | |||
|
|||
@Test | |||
public void AddNodeTest() throws Exception { | |||
public void AddRemoveNodeTest() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a comment as to what is being tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I think we need to run some combination of these actions in multiple tests.
Like:
- add node -> remove node -> add node -> add node
- add node -> add node -> remove node -> add node
- add node -> add node -> add node -> remove node -> remove node -> add node -> add node
and so on.
I understand these are slow tests but we need to catch as many issues as we can as early as we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't test these scenarios in this PR because I need force remove. I will add these test cases to the force remove PR.
|
||
|
||
@Test | ||
public void removeNodeTest() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments please!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// Remove a node from a two node cluster. Verify that a remove operation won't | ||
// cause the loss of redundancy | ||
assertThatThrownBy(() -> r.getLayoutManagementView().removeNode(l2, | ||
getEndpoint(SERVERS.PORT_1))).isInstanceOf(LayoutModificationException.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we also assess what happened to epoch number ? This one tries an attempt where we could lose redundancy. It would be ok if we do not move epochs when such a thing happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an extra assert.
71d8cd2
to
4a13fb2
Compare
Ok, it should pass now. |
log.error("removeNode: Bootstrapping sequencer failed due to exception : ", ie); | ||
throw new UnrecoverableCorfuInterruptedError(ie); | ||
} catch (ExecutionException ee) { | ||
log.error("removeNode: Bootstrapping sequencer failed due to exception : ", ee); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, not handling this doesn't "fix" the problem. At the end of removeNode's execution, what are my guarantees? Is the node removed or not? If the node is guaranteed to be removed, this needs to throw an exception. If not, then how the client should handle it (indeterministic behavior) better be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually does.
This method doesn't provide guarantees. I added more documentation.
Ok, added more documentation. |
I'm not confident about the design of this PR. It seems that since you have information about the failure/success of the reconfiguration, you should present that information to the client whenever possible. @medhavidhawan? |
This is a consequence of the orchestrator stateless design, not this specific PR. Workflows execute optimistically (i.e. no retry logic). The client requesting workflows should retry on failure. Right now this can happen through observing the layout. Since workflows run asynchronously, the orchestrator server returns a workflow id right away. The orchestrator is stateless so it doesn't persist data on workflow status. Based on those two things, the client can observe the layout after it determines that the workflow is no longer running and retry appropriately. |
7df975e
to
4b51cfd
Compare
Results automatically generated by CorfuDB Benchmark Framework to assess the performance of this pull request for commit 4b51cfd. *** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 1 threads, Disk mode An interactive dashboard with Pull Request Performance Metrics for ALL cluster types and numbers of threads in run, is available at: |
@no2chem Addressed all comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments.
@@ -38,7 +38,7 @@ String getConnectionString(int port) { | |||
} | |||
|
|||
@Test | |||
public void AddNodeIT() throws Exception { | |||
public void AddRemoveNodeTest() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should still be named IT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
@Override | ||
public List<Action> getActions() { | ||
return Arrays.asList(new RemoveNode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably you want Collections.singletonList? Using Arrays here on a non-list is a bit odd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
assertThat(r.getLayoutView().getLayout().getEpoch()).isEqualTo(epoch); | ||
} | ||
|
||
void unexceptionalRemove(CorfuRuntime rt, Layout layout, String endpoint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure exactly what unexceptional remove is doing here. I think you want to use assertThatCode(() -> {})..doesNotThrowAnyException();
@no2chem Ok, I think its time to get this in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know its a pain, but we're still missing a few javadoc on LayoutBuilder APIs. I missed these on the last round. In the future if you could check that all public APIs are documented, that would be preferable -- it's easy to miss these things on each pass...
We should also try to have checkstyle catch these violations during CI.
@@ -50,6 +50,11 @@ public LayoutBuilder clearUnResponsiveServers() { | |||
return this; | |||
} | |||
|
|||
public LayoutBuilder setEpoch(long epoch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing javadoc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@@ -72,6 +77,11 @@ public LayoutBuilder removeUnResponsiveServers(Set<String> endpoints) { | |||
return this; | |||
} | |||
|
|||
public LayoutBuilder removeUnResponsiveServer(String endpoint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nonnull, javadoc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
Results automatically generated by CorfuDB Benchmark Framework to assess the performance of this pull request for commit 45d3fd9. *** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 1 threads, Disk mode An interactive dashboard with Pull Request Performance Metrics for ALL cluster types and numbers of threads in run, is available at: |
An implementation of a remove node. This workflow allows the removal of a node from the cluster.
@no2chem addressed all comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, LGTM
Overview
Description:
An implementation of a remove node workflow. This workflow extends the system's capability to downsize the cluster by removing nodes.
Why should this be merged:
This PR and is needed to complete the clustering effort.
Related issue: #1039
Checklist (Definition of Done):