You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "zpinto (via GitHub)" <gi...@apache.org> on 2024/03/01 19:01:14 UTC
[PR] Add ZK multi set with updater to ZKBaseDataAccesor [helix]
zpinto opened a new pull request, #2770:
URL: https://github.com/apache/helix/pull/2770
### Issues
- [ ] Add ZK multi set with updated to ZKBaseDataAccesor. Allows for version based transactional sets.
### Description
This will be used to transactionally update the InstanceOperations of 2 swapping instances in a cluster. It will ensure no more than 1 instance per logicalId can ever hold an assignable InstanceOperation at a time.
### Tests
- [ ] Added testSyncMultiSet
### Changes that Break Backward Compatibility (Optional)
NA
### Commits
- 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"
### Code Quality
- 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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Add ZK multi set with updater to ZKBaseDataAccesor [helix]
Posted by "zpinto (via GitHub)" <gi...@apache.org>.
zpinto commented on PR #2770:
URL: https://github.com/apache/helix/pull/2770#issuecomment-1977283177
This PR is ready to be merged!
Final Commit Message:
Add support for multi/transactional updater to BaseDataAccessor. This will allow transactional version based update of Znodes.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Add ZK multi set with updater to ZKBaseDataAccesor [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #2770:
URL: https://github.com/apache/helix/pull/2770
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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
Re: [PR] Add ZK multi set with updater to ZKBaseDataAccesor [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #2770:
URL: https://github.com/apache/helix/pull/2770#discussion_r1509634366
##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java:
##########
@@ -485,6 +487,56 @@ public AccessResult doUpdate(String path, DataUpdater<T> updater, int options) {
return result;
}
+ /**
+ * transactional sync set
+ */
+ @Override
+ public boolean multiSet(Map<String, DataUpdater<T>> updaterByPath) {
+ AccessResult result = doMultiSet(updaterByPath);
+ return result._retCode == RetCode.OK;
+ }
+
+ private AccessResult doMultiSet(Map<String, DataUpdater<T>> updaterByPath) {
+ AccessResult result = new AccessResult();
+ boolean retry;
+ do {
+ retry = false;
+ try {
+ List<Op> ops = new ArrayList<>();
+ for (Map.Entry<String, DataUpdater<T>> entry : updaterByPath.entrySet()) {
+ String path = entry.getKey();
+ DataUpdater<T> updater = entry.getValue();
+ Stat readStat = new Stat();
+ T oldData = (T) _zkClient.readData(path, readStat);
+ T newData = updater.update(oldData);
+ if (newData != null) {
+ ops.add(Op.setData(path, _zkClient.serialize(newData, path), readStat.getVersion()));
+ }
+ }
+ for (OpResult opResult : _zkClient.multi(ops)) {
+ if (opResult instanceof OpResult.SetDataResult) {
+ DataTree.copyStat(((OpResult.SetDataResult) opResult).getStat(), result._stat);
+ } else if (opResult instanceof OpResult.ErrorResult) {
+ if (((OpResult.ErrorResult) opResult).getErr() == Code.BADVERSION.intValue()) {
Review Comment:
Does this contain the part of update failure due to version mismatch?
##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java:
##########
@@ -485,6 +487,56 @@ public AccessResult doUpdate(String path, DataUpdater<T> updater, int options) {
return result;
}
+ /**
+ * transactional sync set
+ */
+ @Override
+ public boolean multiSet(Map<String, DataUpdater<T>> updaterByPath) {
+ AccessResult result = doMultiSet(updaterByPath);
+ return result._retCode == RetCode.OK;
+ }
+
+ private AccessResult doMultiSet(Map<String, DataUpdater<T>> updaterByPath) {
+ AccessResult result = new AccessResult();
+ boolean retry;
+ do {
+ retry = false;
+ try {
+ List<Op> ops = new ArrayList<>();
+ for (Map.Entry<String, DataUpdater<T>> entry : updaterByPath.entrySet()) {
+ String path = entry.getKey();
+ DataUpdater<T> updater = entry.getValue();
+ Stat readStat = new Stat();
+ T oldData = (T) _zkClient.readData(path, readStat);
+ T newData = updater.update(oldData);
+ if (newData != null) {
+ ops.add(Op.setData(path, _zkClient.serialize(newData, path), readStat.getVersion()));
+ }
+ }
+ for (OpResult opResult : _zkClient.multi(ops)) {
+ if (opResult instanceof OpResult.SetDataResult) {
+ DataTree.copyStat(((OpResult.SetDataResult) opResult).getStat(), result._stat);
+ } else if (opResult instanceof OpResult.ErrorResult) {
+ if (((OpResult.ErrorResult) opResult).getErr() == Code.BADVERSION.intValue()) {
+ retry = true;
+ } else {
+ LOG.error("Failed to update path: " + updaterByPath.keySet());
+ result._retCode = RetCode.ERROR;
+ return result;
+ }
+ }
+ }
+ } catch (Exception e) {
Review Comment:
Would be good to specific the failure exceptions to handle different logic.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
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