You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "patsonluk (via GitHub)" <gi...@apache.org> on 2023/03/08 02:21:40 UTC
[GitHub] [solr] patsonluk opened a new pull request, #1438: SOLR-16690: enqueueUpdate should execute callback even if cmds is empty or noop
patsonluk opened a new pull request, #1438:
URL: https://github.com/apache/solr/pull/1438
https://issues.apache.org/jira/browse/SOLR-16690
# Description
`ZkStateWriter#enqueueUpdate(ClusterState prevState, List<ZkWriteCommand> cmds, ZkWriteCallback callback)` accepts a callback but such callback would not be called if either the cmds is empty or a no-op.
However, this could cause issues in some scenarios, for example in [Overseer$ClusterStateUpdater#run](https://github.com/apache/solr/blob/main/solr/core/src/java/org/apache/solr/cloud/Overseer.java#L373), it calls
```
clusterState = processQueueItem(message, clusterState, zkStateWriter, true,
() ->
{ stateUpdateQueue.remove(processedNodes); processedNodes.clear(); }
);
```
Which it relies on the callback to remove items from the work queue, however if the message does not create an empty `List<ZkWriteCommand>` - for example "DOWNNODE" for PRS enabled collection, then the callback will be ignored hence the `stateUpdateQueue.remove(processedNodes);` would not be run even though it has finished processing the message
# Solution
Execute the callback if cmds is empty or noop
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] patsonluk closed pull request #1438: SOLR-16690: enqueueUpdate should execute callback even if cmds is empty or noop
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk closed pull request #1438: SOLR-16690: enqueueUpdate should execute callback even if cmds is empty or noop
URL: https://github.com/apache/solr/pull/1438
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] patsonluk commented on a diff in pull request #1438: SOLR-16690: enqueueUpdate should execute callback even if cmds is empty or noop
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on code in PR #1438:
URL: https://github.com/apache/solr/pull/1438#discussion_r1134711592
##########
solr/core/src/java/org/apache/solr/cloud/overseer/ZkStateWriter.java:
##########
@@ -139,8 +139,12 @@ public ClusterState enqueueUpdate(
throw new IllegalStateException(
"ZkStateWriter has seen a tragic error, this instance can no longer be used");
}
- if (cmds.isEmpty()) return prevState;
- if (isNoOps(cmds)) return prevState;
+ if (cmds.isEmpty() || isNoOps(cmds)) { // empty and no-ops are valid, should call callback
+ if (callback != null) {
Review Comment:
Added `ZkStateWriterTest#testEnqueueCallback`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] hiteshk25 commented on a diff in pull request #1438: SOLR-16690: enqueueUpdate should execute callback even if cmds is empty or noop
Posted by "hiteshk25 (via GitHub)" <gi...@apache.org>.
hiteshk25 commented on code in PR #1438:
URL: https://github.com/apache/solr/pull/1438#discussion_r1132905423
##########
solr/core/src/java/org/apache/solr/cloud/overseer/ZkStateWriter.java:
##########
@@ -139,8 +139,12 @@ public ClusterState enqueueUpdate(
throw new IllegalStateException(
"ZkStateWriter has seen a tragic error, this instance can no longer be used");
}
- if (cmds.isEmpty()) return prevState;
- if (isNoOps(cmds)) return prevState;
+ if (cmds.isEmpty() || isNoOps(cmds)) { // empty and no-ops are valid, should call callback
+ if (callback != null) {
Review Comment:
Thanks @patsonluk. is it possible to add unit test for this method `enqueueUpdate` ?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] patsonluk commented on pull request #1438: SOLR-16690: enqueueUpdate should execute callback even if cmds is empty or noop
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on PR #1438:
URL: https://github.com/apache/solr/pull/1438#issuecomment-1714685834
Closing this as the proposed change can introduce incorrect behavior. For example, if there's already pending changes in the `ZkStateWriter`, and a no op changes invocation to `enqueueUpdate` can invoke the callback while the actual pending changes are not flushed.
The existing behavior (shortcut no-op/empty ops and skip callback) is not ideal but is technically correct, as the `ZkWriteCallback#onWrite` method has description of `Called by ZkStateWriter if state is flushed to ZK`, since the empty/no-op operation does not trigger any state flush, it should not invoke the callback neither.
This callback might not work in certain edge case, for example if there are a mix of both PRS and non PRS enabled collection, a PRS related operation might invoke `ZkWriteCallback#onWrite` while the operations for non PRS collections are still pending. However, this is probably a rather unlikely case and is beyond the scope of this PR
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org