You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/06/26 04:50:30 UTC
[GitHub] [helix] jiajunwang opened a new pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
jiajunwang opened a new pull request #1123:
URL: https://github.com/apache/helix/pull/1123
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
(#200 - Link your issue number here: You can write "Fixes #XXX")
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
This is to prevent the WAGED rebalancer reads stale assignment records from the previous rebalance pipeline.
For example,
1. Resource A was the only resource. And it is rebalanced by WAGED, then we have a persisted assignment for A.
2. Resource A was reconfigured to using DelayedRebalancer, then we stop the WAGED rebalancer since there is no more resource using WAGED. So the persisted records are still in ZK.
3. Resource A is recreated and using WAGED again. In this case, the previous persisted assignment is no longer valid. We should treat A as a brand new resource instead of considering the stale assignment record.
Moreover, this change will help to clean up the ZK persisted data if no resource is using WAGED.
### Tests
- [X] The following tests are written for this issue:
TestAssignmentMetadataStore, TestWagedRebalancer
- [ ] The following is the result of the "mvn test" command on the appropriate module:
running
### Commits
- [X] My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation (Optional)
- [ ] In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [X] My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-652014255
"allocating over all clusters to instance" what does that mean? Could you please give me an example?
Cheers,
-Jiajun
________________________________
From: kaisun2000 <no...@github.com>
Sent: Tuesday, June 30, 2020 1:03 PM
To: apache/helix <he...@noreply.github.com>
Cc: Jiajun Wang <jj...@linkedin.com>; Author <au...@noreply.github.com>
Subject: Re: [apache/helix] Cleanup the persisted assignment state if no resource is on WAGED rebalancer. (#1123)
But it won't hurt. And IMO, the assumption that we won't change WAGED logic in the future is weak. I prefer to keep it.
As an aside, I recall waged has an advantage as allocating over all clusters to instance. So this is not in this version?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fhelix%2Fpull%2F1123%23issuecomment-652013368&data=02%7C01%7Cjjwang%40linkedin.com%7C6fa9c2a69d514be3a58808d81d3098ee%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637291441844424637&sdata=RooDr9nfa03%2F03oQ97fIe6BJtB3f55prujYogms2P74%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAANYM2BXD7WJPVWJSPTHDE3RZJAHJANCNFSM4OI7NZIA&data=02%7C01%7Cjjwang%40linkedin.com%7C6fa9c2a69d514be3a58808d81d3098ee%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637291441844434630&sdata=9hlYzyVGId9mdz%2BKtuP9cLptJBUDOjJE9KXLedDFBd0%3D&reserved=0>.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-652029624
Thanks for the review : )
Cheers,
-Jiajun
________________________________
From: kaisun2000 <no...@github.com>
Sent: Tuesday, June 30, 2020 1:35 PM
To: apache/helix <he...@noreply.github.com>
Cc: Jiajun Wang <jj...@linkedin.com>; Author <au...@noreply.github.com>
Subject: Re: [apache/helix] Cleanup the persisted assignment state if no resource is on WAGED rebalancer. (#1123)
I see. Thx for the explanation.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fhelix%2Fpull%2F1123%23issuecomment-652028558&data=02%7C01%7Cjjwang%40linkedin.com%7Cdbb1c17cd78b464a1bcb08d81d352672%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637291461399600513&sdata=Z93RzjQpYlnkcVslXtAP6vv%2Fyu7V6fCD6n8c%2FCb4JqU%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAANYM2BK6BORYRYFMGH3RJ3RZJEBRANCNFSM4OI7NZIA&data=02%7C01%7Cjjwang%40linkedin.com%7Cdbb1c17cd78b464a1bcb08d81d352672%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637291461399600513&sdata=dIiU2fAO5ihW2C1QH11MLsXyYtJzQn43UOzvqcnE2C4%3D&reserved=0>.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 edited a comment on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
kaisun2000 edited a comment on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-651996650
My follow-up question is actually that there seems still some potential inconsistency issue here not addressed, namely across controllers over different machine.
The pattern is similar to this https://github.com/apache/helix/pull/1066.
If one controller session expired, and super controller assign another controller to take care the cluster. When first controller come up, his controller pipeline may still run and persists his baseline and best possible again. How do we guard this across java process race condition?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1123:
URL: https://github.com/apache/helix/pull/1123#discussion_r446004357
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/AssignmentMetadataStore.java
##########
@@ -70,7 +70,7 @@ protected AssignmentMetadataStore(BucketDataAccessor bucketDataAccessor, String
_globalBaseline = splitAssignments(baseline);
} catch (ZkNoNodeException ex) {
// Metadata does not exist, so return an empty map
- _globalBaseline = Collections.emptyMap();
+ _globalBaseline = new HashMap<>();
Review comment:
Yes
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1123:
URL: https://github.com/apache/helix/pull/1123#discussion_r447932405
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/AssignmentMetadataStore.java
##########
@@ -95,53 +95,56 @@ protected AssignmentMetadataStore(BucketDataAccessor bucketDataAccessor, String
* @return true if a new baseline was persisted.
* @throws HelixException if the method failed to persist the baseline.
*/
- // TODO: Enhance the return value so it is more intuitive to understand when the persist fails and
- // TODO: when it is skipped.
public synchronized boolean persistBaseline(Map<String, ResourceAssignment> globalBaseline) {
- // TODO: Make the write async?
- // If baseline hasn't changed, skip writing to metadata store
- if (compareAssignments(_globalBaseline, globalBaseline)) {
- return false;
- }
- // Persist to ZK
- HelixProperty combinedAssignments = combineAssignments(BASELINE_KEY, globalBaseline);
- try {
- _dataAccessor.compressedBucketWrite(_baselinePath, combinedAssignments);
- } catch (IOException e) {
- // TODO: Improve failure handling
- throw new HelixException("Failed to persist baseline!", e);
- }
-
- // Update the in-memory reference
- _globalBaseline = globalBaseline;
- return true;
+ return persistAssignment(globalBaseline, getBaseline(), _baselinePath, BASELINE_KEY);
}
/**
* @return true if a new best possible assignment was persisted.
* @throws HelixException if the method failed to persist the baseline.
*/
- // TODO: Enhance the return value so it is more intuitive to understand when the persist fails and
- // TODO: when it is skipped.
public synchronized boolean persistBestPossibleAssignment(
Map<String, ResourceAssignment> bestPossibleAssignment) {
+ return persistAssignment(bestPossibleAssignment, getBestPossibleAssignment(), _bestPossiblePath,
+ BEST_POSSIBLE_KEY);
+ }
+
+ public synchronized void clearAssignmentMetadata() {
+ persistAssignment(Collections.emptyMap(), getBaseline(), _baselinePath, BASELINE_KEY);
+ persistAssignment(Collections.emptyMap(), getBestPossibleAssignment(), _bestPossiblePath,
+ BEST_POSSIBLE_KEY);
+ }
+
+ /**
+ * @param newAssignment
+ * @param cachedAssignment
+ * @param path the path of the assignment record
+ * @param key the key of the assignment in the record
+ * @return true if a new assignment was persisted.
+ */
+ // TODO: Enhance the return value so it is more intuitive to understand when the persist fails and
+ // TODO: when it is skipped.
+ private boolean persistAssignment(Map<String, ResourceAssignment> newAssignment,
+ Map<String, ResourceAssignment> cachedAssignment, String path,
+ String key) {
// TODO: Make the write async?
- // If bestPossibleAssignment hasn't changed, skip writing to metadata store
- if (compareAssignments(_bestPossibleAssignment, bestPossibleAssignment)) {
+ // If the assignment hasn't changed, skip writing to metadata store
+ if (compareAssignments(cachedAssignment, newAssignment)) {
return false;
}
// Persist to ZK
- HelixProperty combinedAssignments =
- combineAssignments(BEST_POSSIBLE_KEY, bestPossibleAssignment);
+ HelixProperty combinedAssignments = combineAssignments(key, newAssignment);
try {
- _dataAccessor.compressedBucketWrite(_bestPossiblePath, combinedAssignments);
+ _dataAccessor.compressedBucketWrite(path, combinedAssignments);
} catch (IOException e) {
// TODO: Improve failure handling
- throw new HelixException("Failed to persist BestPossibleAssignment!", e);
+ throw new HelixException(
Review comment:
The rebalancer will catch it and wrap with rebalancer failure exceptions and add the failure type accordingly.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-652013368
> But it won't hurt. And IMO, the assumption that we won't change WAGED logic in the future is weak. I prefer to keep it.
As an aside, I recall waged has an advantage as allocating over all clusters to instance. So this is not in this version?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1123:
URL: https://github.com/apache/helix/pull/1123#discussion_r447932291
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/AssignmentMetadataStore.java
##########
@@ -70,7 +70,7 @@ protected AssignmentMetadataStore(BucketDataAccessor bucketDataAccessor, String
_globalBaseline = splitAssignments(baseline);
} catch (ZkNoNodeException ex) {
// Metadata does not exist, so return an empty map
- _globalBaseline = Collections.emptyMap();
+ _globalBaseline = new HashMap<>();
Review comment:
It is. In this PR, I changed the logic so the map will be kept. It needs to be modifiable.
Another option is to introduce more logic to handle the corner cases, which I don't prefer.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1123:
URL: https://github.com/apache/helix/pull/1123#discussion_r446004726
##########
File path: helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/TestAssignmentMetadataStore.java
##########
@@ -28,74 +29,50 @@
import org.apache.helix.HelixManagerFactory;
import org.apache.helix.InstanceType;
import org.apache.helix.common.ZkTestBase;
-import org.apache.helix.integration.manager.ClusterControllerManager;
-import org.apache.helix.integration.manager.MockParticipantManager;
+import org.apache.helix.manager.zk.ZkBucketDataAccessor;
import org.apache.helix.model.Partition;
import org.apache.helix.model.ResourceAssignment;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
-
public class TestAssignmentMetadataStore extends ZkTestBase {
- protected static final int NODE_NR = 5;
- protected static final int START_PORT = 12918;
- protected static final String STATE_MODEL = "MasterSlave";
- protected static final String TEST_DB = "TestDB";
- protected static final int _PARTITIONS = 20;
+ private static final int DEFAULT_BUCKET_SIZE = 50 * 1024; // 50KB
+ private static final String BASELINE_KEY = "BASELINE";
+ private static final String BEST_POSSIBLE_KEY = "BEST_POSSIBLE";
+ protected static final String TEST_DB = "TestDB";
protected HelixManager _manager;
protected final String CLASS_NAME = getShortClassName();
protected final String CLUSTER_NAME = CLUSTER_PREFIX + "_" + CLASS_NAME;
- protected MockParticipantManager[] _participants = new MockParticipantManager[NODE_NR];
- protected ClusterControllerManager _controller;
- protected int _replica = 3;
-
private AssignmentMetadataStore _store;
@BeforeClass
- public void beforeClass()
- throws Exception {
+ public void beforeClass() throws Exception {
super.beforeClass();
// setup storage cluster
_gSetupTool.addCluster(CLUSTER_NAME, true);
- _gSetupTool.addResourceToCluster(CLUSTER_NAME, TEST_DB, _PARTITIONS, STATE_MODEL);
- for (int i = 0; i < NODE_NR; i++) {
- String storageNodeName = PARTICIPANT_PREFIX + "_" + (START_PORT + i);
- _gSetupTool.addInstanceToCluster(CLUSTER_NAME, storageNodeName);
- }
- _gSetupTool.rebalanceStorageCluster(CLUSTER_NAME, TEST_DB, _replica);
-
- // start dummy participants
- for (int i = 0; i < NODE_NR; i++) {
- String instanceName = PARTICIPANT_PREFIX + "_" + (START_PORT + i);
- _participants[i] = new MockParticipantManager(ZK_ADDR, CLUSTER_NAME, instanceName);
- _participants[i].syncStart();
- }
-
- // start controller
- String controllerName = CONTROLLER_PREFIX + "_0";
- _controller = new ClusterControllerManager(ZK_ADDR, CLUSTER_NAME, controllerName);
- _controller.syncStart();
// create cluster manager
_manager = HelixManagerFactory
.getZKHelixManager(CLUSTER_NAME, "Admin", InstanceType.ADMINISTRATOR, ZK_ADDR);
_manager.connect();
- // create AssignmentMetadataStore
- _store = new AssignmentMetadataStore(_manager.getMetadataStoreConnectionString(),
- _manager.getClusterName());
+ // Create AssignmentMetadataStore. No version clean up to ensure the test result is stable.
+ _store = new AssignmentMetadataStore(
+ new ZkBucketDataAccessor(_manager.getMetadataStoreConnectionString(), DEFAULT_BUCKET_SIZE,
+ Integer.MAX_VALUE), _manager.getClusterName());
}
@AfterClass
public void afterClass() {
if (_store != null) {
_store.close();
}
+ _baseAccessor.remove("/" + CLUSTER_NAME + "/ASSIGNMENT_METADATA", AccessOption.PERSISTENT);
Review comment:
Yeah, I think we can just remove the cluster.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-652008416
> > You are wrong.
> >
> > 1. Both WAGED and the traditional rebalancers are for each cluster.
> > 2. synchronized is not a must if we make the assumption that it will only be used in the WAGED rebalancer. And we are not going to change the WAGED rebalancer.
> > 3. Having this to protect is a nice-to-have thing. So do you suggest we remove it? I think it won't hurt.
>
> So the invariant is that each cluster has its own copy of persisted baseline and best-possible. If this is clear, removing sync seems to be safe.
But it won't hurt. And IMO, the assumption that we won't change WAGED logic in the future is weak. I prefer to keep it.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-652003864
> One high level question. `public synchronized boolean persistBaseline` or bestPossible, the synchronized is to make sure that with in a process, the persisting to ZK is guarded. Is there a case that in a single controller java process, there will be more than one waged pipeline running? My understanding is that for CRUSH full auto, we have per cluster controller pipeline. But for waged, they is only one for all clusters? Am I wrong here?
You are wrong.
1. Both WAGED and the traditional rebalancers are for each cluster.
2. synchronized is not a must if we make the assumption that it will only be used in the WAGED rebalancer. And we are not going to change the WAGED rebalancer.
3. Having this to protect is a nice-to-have thing. So do you suggest we remove it? I think it won't hurt.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-652017384
Whichever wins is fine. Since the next pipeline running will take the cached result and keep calculating based on there.
* If A wins, then A keeps calculating based on its local state.
* If B wins, then because A cache the result after write, it will still calculate based on its local state. And the next pipeline will overwrite B's output. And we are good.
In this case, we don't really need B's result.
Cheers,
-Jiajun
________________________________
From: kaisun2000 <no...@github.com>
Sent: Tuesday, June 30, 2020 1:07 PM
To: apache/helix <he...@noreply.github.com>
Cc: Jiajun Wang <jj...@linkedin.com>; Author <au...@noreply.github.com>
Subject: Re: [apache/helix] Cleanup the persisted assignment state if no resource is on WAGED rebalancer. (#1123)
Please note that we have an MVCC implemented in the bucket accessor to ensure cross node consistency.
I will try to examine the bucket accessor a little bit more carefully. Here is the question:
Say controller A is for resource R allocation. Then controller A session expires, controller B take care of resource B. Later when A is back, for a while A and B would run at the same time. When serializing say baseline for resource R, A wins over B, is it going to be a problem for this resource as B would later be controller for R.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fhelix%2Fpull%2F1123%23issuecomment-652015520&data=02%7C01%7Cjjwang%40linkedin.com%7C61ccb448e537425425a008d81d313072%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637291444385780233&sdata=VIdFasrPljxVkFFnODjJGoxHpYl%2FBmYSxQF5xNHHmzA%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAANYM2AOROJKY37OFQ3OTXTRZJAXHANCNFSM4OI7NZIA&data=02%7C01%7Cjjwang%40linkedin.com%7C61ccb448e537425425a008d81d313072%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637291444385780233&sdata=p2xLzQ1lM%2FPQ2xxMpmqxeMS3cHop%2BzaFqgxPOdOXnc4%3D&reserved=0>.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1123:
URL: https://github.com/apache/helix/pull/1123#discussion_r446005583
##########
File path: helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/TestAssignmentMetadataStore.java
##########
@@ -172,13 +197,17 @@ public void testAssignmentCache() {
/**
* Returns a list of existing version numbers only.
+ *
* @param metadataType
* @return
*/
private List<String> getExistingVersionNumbers(String metadataType) {
List<String> children = _baseAccessor
.getChildNames("/" + CLUSTER_NAME + "/ASSIGNMENT_METADATA/" + metadataType,
AccessOption.PERSISTENT);
+ if (children == null) {
+ children = Collections.EMPTY_LIST;
Review comment:
It doesn't hurt. What if we want to add something to the list in the future? This is test, so I would like to reduce the code branch as much as possible. The only cost is a little bit more computing. But it is a test.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-652015520
> Please note that we have an MVCC implemented in the bucket accessor to ensure cross node consistency.
I will try to examine the bucket accessor a little bit more carefully. Here is the question:
Say controller A is for resource R allocation. Then controller A session expires, controller B take care of resource B. Later when A is back, for a while A and B would run at the same time. When serializing say baseline for resource R, A wins over B, is it going to be a problem for this resource as B would later be controller for R.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1123:
URL: https://github.com/apache/helix/pull/1123#discussion_r447913951
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/AssignmentMetadataStore.java
##########
@@ -70,7 +70,7 @@ protected AssignmentMetadataStore(BucketDataAccessor bucketDataAccessor, String
_globalBaseline = splitAssignments(baseline);
} catch (ZkNoNodeException ex) {
// Metadata does not exist, so return an empty map
- _globalBaseline = Collections.emptyMap();
+ _globalBaseline = new HashMap<>();
Review comment:
This change is non-essential, right?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-652006415
> My follow-up question is actually that there seems still some potential inconsistency issue here not addressed, namely across controllers over different machine.
>
> The pattern is similar to this #1066.
>
> If one controller session expired, and super controller assign another controller to take care the cluster. When first controller come up, his controller pipeline may still run and persists his baseline and best possible again. How do we guard this across java process race condition?
>
> This is a pattern of similar problems. We should think a little bit more generic and address them all long term.
Please note that we have an MVCC implemented in the bucket accessor to ensure cross node consistency.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on a change in pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1123:
URL: https://github.com/apache/helix/pull/1123#discussion_r447922995
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/AssignmentMetadataStore.java
##########
@@ -95,53 +95,56 @@ protected AssignmentMetadataStore(BucketDataAccessor bucketDataAccessor, String
* @return true if a new baseline was persisted.
* @throws HelixException if the method failed to persist the baseline.
*/
- // TODO: Enhance the return value so it is more intuitive to understand when the persist fails and
- // TODO: when it is skipped.
public synchronized boolean persistBaseline(Map<String, ResourceAssignment> globalBaseline) {
- // TODO: Make the write async?
- // If baseline hasn't changed, skip writing to metadata store
- if (compareAssignments(_globalBaseline, globalBaseline)) {
- return false;
- }
- // Persist to ZK
- HelixProperty combinedAssignments = combineAssignments(BASELINE_KEY, globalBaseline);
- try {
- _dataAccessor.compressedBucketWrite(_baselinePath, combinedAssignments);
- } catch (IOException e) {
- // TODO: Improve failure handling
- throw new HelixException("Failed to persist baseline!", e);
- }
-
- // Update the in-memory reference
- _globalBaseline = globalBaseline;
- return true;
+ return persistAssignment(globalBaseline, getBaseline(), _baselinePath, BASELINE_KEY);
}
/**
* @return true if a new best possible assignment was persisted.
* @throws HelixException if the method failed to persist the baseline.
*/
- // TODO: Enhance the return value so it is more intuitive to understand when the persist fails and
- // TODO: when it is skipped.
public synchronized boolean persistBestPossibleAssignment(
Map<String, ResourceAssignment> bestPossibleAssignment) {
+ return persistAssignment(bestPossibleAssignment, getBestPossibleAssignment(), _bestPossiblePath,
+ BEST_POSSIBLE_KEY);
+ }
+
+ public synchronized void clearAssignmentMetadata() {
+ persistAssignment(Collections.emptyMap(), getBaseline(), _baselinePath, BASELINE_KEY);
+ persistAssignment(Collections.emptyMap(), getBestPossibleAssignment(), _bestPossiblePath,
+ BEST_POSSIBLE_KEY);
+ }
+
+ /**
+ * @param newAssignment
+ * @param cachedAssignment
+ * @param path the path of the assignment record
+ * @param key the key of the assignment in the record
+ * @return true if a new assignment was persisted.
+ */
+ // TODO: Enhance the return value so it is more intuitive to understand when the persist fails and
+ // TODO: when it is skipped.
+ private boolean persistAssignment(Map<String, ResourceAssignment> newAssignment,
+ Map<String, ResourceAssignment> cachedAssignment, String path,
+ String key) {
// TODO: Make the write async?
- // If bestPossibleAssignment hasn't changed, skip writing to metadata store
- if (compareAssignments(_bestPossibleAssignment, bestPossibleAssignment)) {
+ // If the assignment hasn't changed, skip writing to metadata store
+ if (compareAssignments(cachedAssignment, newAssignment)) {
return false;
}
// Persist to ZK
- HelixProperty combinedAssignments =
- combineAssignments(BEST_POSSIBLE_KEY, bestPossibleAssignment);
+ HelixProperty combinedAssignments = combineAssignments(key, newAssignment);
try {
- _dataAccessor.compressedBucketWrite(_bestPossiblePath, combinedAssignments);
+ _dataAccessor.compressedBucketWrite(path, combinedAssignments);
} catch (IOException e) {
// TODO: Improve failure handling
- throw new HelixException("Failed to persist BestPossibleAssignment!", e);
+ throw new HelixException(
Review comment:
What is going to catch this one?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 edited a comment on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
kaisun2000 edited a comment on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-651996650
My follow-up question is actually that there seems still some potential inconsistency issue here not addressed, namely across controllers over different machine.
The pattern is similar to this https://github.com/apache/helix/pull/1066.
If one controller session expired, and super controller assign another controller to take care the cluster. When first controller come up, his controller pipeline may still run and persists his baseline and best possible again. How do we guard this across java process race condition?
This is a pattern of similar problems. We should think a little bit more generic and address them all long term.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-652083218
This PR is ready to be merged, approved by @pkuwm
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-652028558
I see. Thx for the explanation.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1123:
URL: https://github.com/apache/helix/pull/1123#discussion_r447932405
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/AssignmentMetadataStore.java
##########
@@ -95,53 +95,56 @@ protected AssignmentMetadataStore(BucketDataAccessor bucketDataAccessor, String
* @return true if a new baseline was persisted.
* @throws HelixException if the method failed to persist the baseline.
*/
- // TODO: Enhance the return value so it is more intuitive to understand when the persist fails and
- // TODO: when it is skipped.
public synchronized boolean persistBaseline(Map<String, ResourceAssignment> globalBaseline) {
- // TODO: Make the write async?
- // If baseline hasn't changed, skip writing to metadata store
- if (compareAssignments(_globalBaseline, globalBaseline)) {
- return false;
- }
- // Persist to ZK
- HelixProperty combinedAssignments = combineAssignments(BASELINE_KEY, globalBaseline);
- try {
- _dataAccessor.compressedBucketWrite(_baselinePath, combinedAssignments);
- } catch (IOException e) {
- // TODO: Improve failure handling
- throw new HelixException("Failed to persist baseline!", e);
- }
-
- // Update the in-memory reference
- _globalBaseline = globalBaseline;
- return true;
+ return persistAssignment(globalBaseline, getBaseline(), _baselinePath, BASELINE_KEY);
}
/**
* @return true if a new best possible assignment was persisted.
* @throws HelixException if the method failed to persist the baseline.
*/
- // TODO: Enhance the return value so it is more intuitive to understand when the persist fails and
- // TODO: when it is skipped.
public synchronized boolean persistBestPossibleAssignment(
Map<String, ResourceAssignment> bestPossibleAssignment) {
+ return persistAssignment(bestPossibleAssignment, getBestPossibleAssignment(), _bestPossiblePath,
+ BEST_POSSIBLE_KEY);
+ }
+
+ public synchronized void clearAssignmentMetadata() {
+ persistAssignment(Collections.emptyMap(), getBaseline(), _baselinePath, BASELINE_KEY);
+ persistAssignment(Collections.emptyMap(), getBestPossibleAssignment(), _bestPossiblePath,
+ BEST_POSSIBLE_KEY);
+ }
+
+ /**
+ * @param newAssignment
+ * @param cachedAssignment
+ * @param path the path of the assignment record
+ * @param key the key of the assignment in the record
+ * @return true if a new assignment was persisted.
+ */
+ // TODO: Enhance the return value so it is more intuitive to understand when the persist fails and
+ // TODO: when it is skipped.
+ private boolean persistAssignment(Map<String, ResourceAssignment> newAssignment,
+ Map<String, ResourceAssignment> cachedAssignment, String path,
+ String key) {
// TODO: Make the write async?
- // If bestPossibleAssignment hasn't changed, skip writing to metadata store
- if (compareAssignments(_bestPossibleAssignment, bestPossibleAssignment)) {
+ // If the assignment hasn't changed, skip writing to metadata store
+ if (compareAssignments(cachedAssignment, newAssignment)) {
return false;
}
// Persist to ZK
- HelixProperty combinedAssignments =
- combineAssignments(BEST_POSSIBLE_KEY, bestPossibleAssignment);
+ HelixProperty combinedAssignments = combineAssignments(key, newAssignment);
try {
- _dataAccessor.compressedBucketWrite(_bestPossiblePath, combinedAssignments);
+ _dataAccessor.compressedBucketWrite(path, combinedAssignments);
} catch (IOException e) {
// TODO: Improve failure handling
- throw new HelixException("Failed to persist BestPossibleAssignment!", e);
+ throw new HelixException(
Review comment:
The rebalancer will do it.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1123:
URL: https://github.com/apache/helix/pull/1123#discussion_r446004914
##########
File path: helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/TestAssignmentMetadataStore.java
##########
@@ -107,55 +84,103 @@ public void afterClass() {
*/
@Test
public void testReadEmptyBaseline() {
- Map<String, ResourceAssignment> baseline = _store.getBaseline();
- Assert.assertTrue(baseline.isEmpty());
+ // This should be the first test. Assert there is no record in ZK.
+ // Check that only one version exists
+ Assert.assertEquals(getExistingVersionNumbers(BASELINE_KEY).size(), 0);
+ Assert.assertEquals(getExistingVersionNumbers(BEST_POSSIBLE_KEY).size(), 0);
+ // Read from cache and the result is empty.
+ Assert.assertTrue(_store.getBaseline().isEmpty());
+ Assert.assertTrue(_store.getBestPossibleAssignment().isEmpty());
}
/**
* Test that if the old assignment and new assignment are the same,
*/
@Test(dependsOnMethods = "testReadEmptyBaseline")
public void testAvoidingRedundantWrite() {
- String baselineKey = "BASELINE";
- String bestPossibleKey = "BEST_POSSIBLE";
-
Map<String, ResourceAssignment> dummyAssignment = getDummyAssignment();
// Call persist functions
_store.persistBaseline(dummyAssignment);
_store.persistBestPossibleAssignment(dummyAssignment);
// Check that only one version exists
- List<String> baselineVersions = getExistingVersionNumbers(baselineKey);
- List<String> bestPossibleVersions = getExistingVersionNumbers(bestPossibleKey);
- Assert.assertEquals(baselineVersions.size(), 1);
- Assert.assertEquals(bestPossibleVersions.size(), 1);
+ Assert.assertEquals(getExistingVersionNumbers(BASELINE_KEY).size(), 1);
+ Assert.assertEquals(getExistingVersionNumbers(BEST_POSSIBLE_KEY).size(), 1);
// Call persist functions again
_store.persistBaseline(dummyAssignment);
_store.persistBestPossibleAssignment(dummyAssignment);
// Check that only one version exists still
- baselineVersions = getExistingVersionNumbers(baselineKey);
- bestPossibleVersions = getExistingVersionNumbers(bestPossibleKey);
- Assert.assertEquals(baselineVersions.size(), 1);
- Assert.assertEquals(bestPossibleVersions.size(), 1);
+ Assert.assertEquals(getExistingVersionNumbers(BASELINE_KEY).size(), 1);
+ Assert.assertEquals(getExistingVersionNumbers(BEST_POSSIBLE_KEY).size(), 1);
}
- @Test
+ @Test(dependsOnMethods = "testAvoidingRedundantWrite")
public void testAssignmentCache() {
Map<String, ResourceAssignment> dummyAssignment = getDummyAssignment();
// Call persist functions
_store.persistBaseline(dummyAssignment);
_store.persistBestPossibleAssignment(dummyAssignment);
+ // Check that only one version exists
+ Assert.assertEquals(getExistingVersionNumbers(BASELINE_KEY).size(), 1);
+ Assert.assertEquals(getExistingVersionNumbers(BEST_POSSIBLE_KEY).size(), 1);
+
+ // Same data in cache
Assert.assertEquals(_store._bestPossibleAssignment, dummyAssignment);
Assert.assertEquals(_store._globalBaseline, dummyAssignment);
+ dummyAssignment.values().stream().forEach(assignment -> {
+ assignment.addReplicaMap(new Partition("foo"), Collections.emptyMap());
+ });
+
+ // Call persist functions
+ _store.persistBaseline(dummyAssignment);
+ _store.persistBestPossibleAssignment(dummyAssignment);
+
+ // Check that two versions exist
+ Assert.assertEquals(getExistingVersionNumbers(BASELINE_KEY).size(), 2);
+ Assert.assertEquals(getExistingVersionNumbers(BEST_POSSIBLE_KEY).size(), 2);
+
+ // Same data in cache
+ Assert.assertEquals(_store._bestPossibleAssignment, dummyAssignment);
+ Assert.assertEquals(_store._globalBaseline, dummyAssignment);
+
+ // Clear cache
_store.reset();
Assert.assertEquals(_store._bestPossibleAssignment, null);
Assert.assertEquals(_store._globalBaseline, null);
+
+ // Check the persisted data is not changed.
+ Assert.assertEquals(getExistingVersionNumbers(BASELINE_KEY).size(), 2);
+ Assert.assertEquals(getExistingVersionNumbers(BEST_POSSIBLE_KEY).size(), 2);
+ }
+
+ @Test(dependsOnMethods = "testAssignmentCache")
+ void testClearAssignment() {
+ // Check the persisted data is not empty
+ List<String> baselineVersions = getExistingVersionNumbers(BASELINE_KEY);
+ List<String> bestPossibleVersions = getExistingVersionNumbers(BEST_POSSIBLE_KEY);
+ int baselineVersionCount = baselineVersions.size();
+ int bestPossibleVersionCount = bestPossibleVersions.size();
+ Assert.assertTrue(baselineVersionCount > 0);
+ Assert.assertTrue(bestPossibleVersionCount > 0);
+
+ _store.clearAssignmentMetadata();
+
+ // 1. cache is cleaned up
+ Assert.assertEquals(_store._bestPossibleAssignment, Collections.emptyMap());
+ Assert.assertEquals(_store._globalBaseline, Collections.emptyMap());
+ // 2. refresh the cache and then read from ZK again to ensure the persisted assignments is empty
+ _store.reset();
+ Assert.assertEquals(_store.getBaseline(), Collections.emptyMap());
Review comment:
No difference. Same thing.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang merged pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1123:
URL: https://github.com/apache/helix/pull/1123
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1123:
URL: https://github.com/apache/helix/pull/1123#discussion_r446002584
##########
File path: helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/TestAssignmentMetadataStore.java
##########
@@ -28,74 +29,50 @@
import org.apache.helix.HelixManagerFactory;
import org.apache.helix.InstanceType;
import org.apache.helix.common.ZkTestBase;
-import org.apache.helix.integration.manager.ClusterControllerManager;
-import org.apache.helix.integration.manager.MockParticipantManager;
+import org.apache.helix.manager.zk.ZkBucketDataAccessor;
import org.apache.helix.model.Partition;
import org.apache.helix.model.ResourceAssignment;
import org.testng.Assert;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
-
public class TestAssignmentMetadataStore extends ZkTestBase {
- protected static final int NODE_NR = 5;
- protected static final int START_PORT = 12918;
- protected static final String STATE_MODEL = "MasterSlave";
- protected static final String TEST_DB = "TestDB";
- protected static final int _PARTITIONS = 20;
+ private static final int DEFAULT_BUCKET_SIZE = 50 * 1024; // 50KB
+ private static final String BASELINE_KEY = "BASELINE";
+ private static final String BEST_POSSIBLE_KEY = "BEST_POSSIBLE";
+ protected static final String TEST_DB = "TestDB";
protected HelixManager _manager;
protected final String CLASS_NAME = getShortClassName();
protected final String CLUSTER_NAME = CLUSTER_PREFIX + "_" + CLASS_NAME;
- protected MockParticipantManager[] _participants = new MockParticipantManager[NODE_NR];
- protected ClusterControllerManager _controller;
- protected int _replica = 3;
-
private AssignmentMetadataStore _store;
@BeforeClass
- public void beforeClass()
- throws Exception {
+ public void beforeClass() throws Exception {
super.beforeClass();
// setup storage cluster
_gSetupTool.addCluster(CLUSTER_NAME, true);
- _gSetupTool.addResourceToCluster(CLUSTER_NAME, TEST_DB, _PARTITIONS, STATE_MODEL);
- for (int i = 0; i < NODE_NR; i++) {
- String storageNodeName = PARTICIPANT_PREFIX + "_" + (START_PORT + i);
- _gSetupTool.addInstanceToCluster(CLUSTER_NAME, storageNodeName);
- }
- _gSetupTool.rebalanceStorageCluster(CLUSTER_NAME, TEST_DB, _replica);
-
- // start dummy participants
- for (int i = 0; i < NODE_NR; i++) {
- String instanceName = PARTICIPANT_PREFIX + "_" + (START_PORT + i);
- _participants[i] = new MockParticipantManager(ZK_ADDR, CLUSTER_NAME, instanceName);
- _participants[i].syncStart();
- }
-
- // start controller
- String controllerName = CONTROLLER_PREFIX + "_0";
- _controller = new ClusterControllerManager(ZK_ADDR, CLUSTER_NAME, controllerName);
- _controller.syncStart();
// create cluster manager
_manager = HelixManagerFactory
.getZKHelixManager(CLUSTER_NAME, "Admin", InstanceType.ADMINISTRATOR, ZK_ADDR);
_manager.connect();
- // create AssignmentMetadataStore
- _store = new AssignmentMetadataStore(_manager.getMetadataStoreConnectionString(),
- _manager.getClusterName());
+ // Create AssignmentMetadataStore. No version clean up to ensure the test result is stable.
+ _store = new AssignmentMetadataStore(
+ new ZkBucketDataAccessor(_manager.getMetadataStoreConnectionString(), DEFAULT_BUCKET_SIZE,
+ Integer.MAX_VALUE), _manager.getClusterName());
}
@AfterClass
public void afterClass() {
if (_store != null) {
_store.close();
}
+ _baseAccessor.remove("/" + CLUSTER_NAME + "/ASSIGNMENT_METADATA", AccessOption.PERSISTENT);
Review comment:
How about removing the whole cluster deleteRecursively("/CLUSTER_NAME")?
##########
File path: helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/TestAssignmentMetadataStore.java
##########
@@ -172,13 +197,17 @@ public void testAssignmentCache() {
/**
* Returns a list of existing version numbers only.
+ *
* @param metadataType
* @return
*/
private List<String> getExistingVersionNumbers(String metadataType) {
List<String> children = _baseAccessor
.getChildNames("/" + CLUSTER_NAME + "/ASSIGNMENT_METADATA/" + metadataType,
AccessOption.PERSISTENT);
+ if (children == null) {
+ children = Collections.EMPTY_LIST;
Review comment:
No need to execute following removes ops? Just return the empty list?
##########
File path: helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/TestAssignmentMetadataStore.java
##########
@@ -107,55 +84,103 @@ public void afterClass() {
*/
@Test
public void testReadEmptyBaseline() {
- Map<String, ResourceAssignment> baseline = _store.getBaseline();
- Assert.assertTrue(baseline.isEmpty());
+ // This should be the first test. Assert there is no record in ZK.
+ // Check that only one version exists
+ Assert.assertEquals(getExistingVersionNumbers(BASELINE_KEY).size(), 0);
+ Assert.assertEquals(getExistingVersionNumbers(BEST_POSSIBLE_KEY).size(), 0);
+ // Read from cache and the result is empty.
+ Assert.assertTrue(_store.getBaseline().isEmpty());
+ Assert.assertTrue(_store.getBestPossibleAssignment().isEmpty());
}
/**
* Test that if the old assignment and new assignment are the same,
*/
@Test(dependsOnMethods = "testReadEmptyBaseline")
public void testAvoidingRedundantWrite() {
- String baselineKey = "BASELINE";
- String bestPossibleKey = "BEST_POSSIBLE";
-
Map<String, ResourceAssignment> dummyAssignment = getDummyAssignment();
// Call persist functions
_store.persistBaseline(dummyAssignment);
_store.persistBestPossibleAssignment(dummyAssignment);
// Check that only one version exists
- List<String> baselineVersions = getExistingVersionNumbers(baselineKey);
- List<String> bestPossibleVersions = getExistingVersionNumbers(bestPossibleKey);
- Assert.assertEquals(baselineVersions.size(), 1);
- Assert.assertEquals(bestPossibleVersions.size(), 1);
+ Assert.assertEquals(getExistingVersionNumbers(BASELINE_KEY).size(), 1);
+ Assert.assertEquals(getExistingVersionNumbers(BEST_POSSIBLE_KEY).size(), 1);
// Call persist functions again
_store.persistBaseline(dummyAssignment);
_store.persistBestPossibleAssignment(dummyAssignment);
// Check that only one version exists still
- baselineVersions = getExistingVersionNumbers(baselineKey);
- bestPossibleVersions = getExistingVersionNumbers(bestPossibleKey);
- Assert.assertEquals(baselineVersions.size(), 1);
- Assert.assertEquals(bestPossibleVersions.size(), 1);
+ Assert.assertEquals(getExistingVersionNumbers(BASELINE_KEY).size(), 1);
+ Assert.assertEquals(getExistingVersionNumbers(BEST_POSSIBLE_KEY).size(), 1);
}
- @Test
+ @Test(dependsOnMethods = "testAvoidingRedundantWrite")
public void testAssignmentCache() {
Map<String, ResourceAssignment> dummyAssignment = getDummyAssignment();
// Call persist functions
_store.persistBaseline(dummyAssignment);
_store.persistBestPossibleAssignment(dummyAssignment);
+ // Check that only one version exists
+ Assert.assertEquals(getExistingVersionNumbers(BASELINE_KEY).size(), 1);
+ Assert.assertEquals(getExistingVersionNumbers(BEST_POSSIBLE_KEY).size(), 1);
+
+ // Same data in cache
Assert.assertEquals(_store._bestPossibleAssignment, dummyAssignment);
Assert.assertEquals(_store._globalBaseline, dummyAssignment);
+ dummyAssignment.values().stream().forEach(assignment -> {
+ assignment.addReplicaMap(new Partition("foo"), Collections.emptyMap());
+ });
+
+ // Call persist functions
+ _store.persistBaseline(dummyAssignment);
+ _store.persistBestPossibleAssignment(dummyAssignment);
+
+ // Check that two versions exist
+ Assert.assertEquals(getExistingVersionNumbers(BASELINE_KEY).size(), 2);
+ Assert.assertEquals(getExistingVersionNumbers(BEST_POSSIBLE_KEY).size(), 2);
+
+ // Same data in cache
+ Assert.assertEquals(_store._bestPossibleAssignment, dummyAssignment);
+ Assert.assertEquals(_store._globalBaseline, dummyAssignment);
+
+ // Clear cache
_store.reset();
Assert.assertEquals(_store._bestPossibleAssignment, null);
Assert.assertEquals(_store._globalBaseline, null);
+
+ // Check the persisted data is not changed.
+ Assert.assertEquals(getExistingVersionNumbers(BASELINE_KEY).size(), 2);
+ Assert.assertEquals(getExistingVersionNumbers(BEST_POSSIBLE_KEY).size(), 2);
+ }
+
+ @Test(dependsOnMethods = "testAssignmentCache")
+ void testClearAssignment() {
+ // Check the persisted data is not empty
+ List<String> baselineVersions = getExistingVersionNumbers(BASELINE_KEY);
+ List<String> bestPossibleVersions = getExistingVersionNumbers(BEST_POSSIBLE_KEY);
+ int baselineVersionCount = baselineVersions.size();
+ int bestPossibleVersionCount = bestPossibleVersions.size();
+ Assert.assertTrue(baselineVersionCount > 0);
+ Assert.assertTrue(bestPossibleVersionCount > 0);
+
+ _store.clearAssignmentMetadata();
+
+ // 1. cache is cleaned up
+ Assert.assertEquals(_store._bestPossibleAssignment, Collections.emptyMap());
+ Assert.assertEquals(_store._globalBaseline, Collections.emptyMap());
+ // 2. refresh the cache and then read from ZK again to ensure the persisted assignments is empty
+ _store.reset();
+ Assert.assertEquals(_store.getBaseline(), Collections.emptyMap());
Review comment:
Nit, `Assert.assertTrue(_store.getBaseline().isEmpty());`? Or is it because `_store.getBaseline()` may return null?
##########
File path: helix-core/src/main/java/org/apache/helix/controller/rebalancer/waged/AssignmentMetadataStore.java
##########
@@ -70,7 +70,7 @@ protected AssignmentMetadataStore(BucketDataAccessor bucketDataAccessor, String
_globalBaseline = splitAssignments(baseline);
} catch (ZkNoNodeException ex) {
// Metadata does not exist, so return an empty map
- _globalBaseline = Collections.emptyMap();
+ _globalBaseline = new HashMap<>();
Review comment:
New elements need to add to this empty map?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-652006981
> You are wrong.
>
> 1. Both WAGED and the traditional rebalancers are for each cluster.
> 2. synchronized is not a must if we make the assumption that it will only be used in the WAGED rebalancer. And we are not going to change the WAGED rebalancer.
> 3. Having this to protect is a nice-to-have thing. So do you suggest we remove it? I think it won't hurt.
So the invariant is that each cluster has its own copy of persisted baseline and best-possible. If this is clear, removing sync seems to be safe.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1123:
URL: https://github.com/apache/helix/pull/1123#issuecomment-651996650
My follow-up question is actually that there seems still some potential inconsistency issue here not address across controllers over different machine.
The pattern is similar to this https://github.com/apache/helix/pull/1066.
If one controller session expired, and super controller assign another controller to take care the cluster. When first controller come up, his controller pipeline may still run and persists his baseline and best possible again. How do we guard this across java process race condition?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1123: Cleanup the persisted assignment state if no resource is on WAGED rebalancer.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1123:
URL: https://github.com/apache/helix/pull/1123#discussion_r446005583
##########
File path: helix-core/src/test/java/org/apache/helix/controller/rebalancer/waged/TestAssignmentMetadataStore.java
##########
@@ -172,13 +197,17 @@ public void testAssignmentCache() {
/**
* Returns a list of existing version numbers only.
+ *
* @param metadataType
* @return
*/
private List<String> getExistingVersionNumbers(String metadataType) {
List<String> children = _baseAccessor
.getChildNames("/" + CLUSTER_NAME + "/ASSIGNMENT_METADATA/" + metadataType,
AccessOption.PERSISTENT);
+ if (children == null) {
+ children = Collections.EMPTY_LIST;
Review comment:
It doesn't hurt. What if we want to add something to the list in the future?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org