Copilot commented on code in PR #16900:
URL: https://github.com/apache/pinot/pull/16900#discussion_r2380034298
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/IdealStateGroupCommit.java:
##########
@@ -258,6 +259,24 @@ public Boolean call() {
} catch (ZkBadVersionException e) {
LOGGER.warn("Version changed while updating ideal state for
resource: {}", resourceName);
return false;
+ } catch (ZkInterruptedException e) {
+ LOGGER.warn("Caught ZkInterruptedException while updating
resource: {}, verifying...",
+ resourceName);
+ IdealState writtenIdealState =
dataAccessor.getProperty(idealStateKey);
+ if (writtenIdealState == null) {
+ LOGGER.warn("IdealState not found after interrupt for
resource: {}", resourceName);
+ return false;
+ }
+ int previousVersion = idealState.getRecord().getVersion();
+ if (writtenIdealState.getRecord().getVersion() <= previousVersion
+ || !writtenIdealState.equals(updatedIdealState)) {
+ LOGGER.warn("New IdealState was not written successfully for
resource: {}", resourceName);
+ return false;
+ } else {
+ LOGGER.info("IdealState was written successfully after
interrupt for resource: {}", resourceName);
+ idealStateWrapper._idealState = updatedIdealState;
Review Comment:
The wrapper should be updated with the `writtenIdealState` that was
successfully read back from ZooKeeper, not the `updatedIdealState` that we
attempted to write. This ensures the wrapper reflects the actual persisted
state and its version number.
```suggestion
idealStateWrapper._idealState = writtenIdealState;
```
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/IdealStateGroupCommit.java:
##########
@@ -258,6 +259,24 @@ public Boolean call() {
} catch (ZkBadVersionException e) {
LOGGER.warn("Version changed while updating ideal state for
resource: {}", resourceName);
return false;
+ } catch (ZkInterruptedException e) {
+ LOGGER.warn("Caught ZkInterruptedException while updating
resource: {}, verifying...",
+ resourceName);
+ IdealState writtenIdealState =
dataAccessor.getProperty(idealStateKey);
+ if (writtenIdealState == null) {
+ LOGGER.warn("IdealState not found after interrupt for
resource: {}", resourceName);
+ return false;
+ }
+ int previousVersion = idealState.getRecord().getVersion();
+ if (writtenIdealState.getRecord().getVersion() <= previousVersion
Review Comment:
The version comparison uses `<=` which could incorrectly fail verification
if the version wrapped around or if ZooKeeper assigned the same version. Since
version advancement is the primary indicator of a successful write, this should
use `<` to only fail when the version hasn't advanced at all.
```suggestion
if (writtenIdealState.getRecord().getVersion() <
previousVersion
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
