dinoocch opened a new pull request, #14214: URL: https://github.com/apache/pinot/pull/14214
This attempts to fix two issues I believe are introduced in https://github.com/apache/pinot/pull/13976/ I think both can cause a table's idealstate to be updated into an unintended state. We luckily found this issue very quickly when upgrading thanks to consistent push failures. My understanding of the code is that it intends to: * Read the idealstate once for a (potential) batch of updates * Apply all the outstanding updates to the znode * Write to zookeeper once for the whole batch ### Issue 1 - Missing updates Previously the code calculated the idealstate value and blindly set it in the `update`. Particularly, if the idealstate is updated between the first and second reads you may unintentionally clobber the update. ```java // In commit, any -> updatedIdealState is used updateIdealState(helixManager, resourceName, anyIdealState -> finalUpdatedIdealState, retryPolicy, noChangeOk); // Then in updateIdealState we read the znode again and use the version of that read in the update IdealState idealState = dataAccessor.getProperty(idealStateKey); ... if (dataAccessor.getBaseDataAccessor() .set(idealStateKey.getPath(), updatedZNRecord, idealState.getRecord().getVersion(), AccessOption.PERSISTENT)) ``` ### Issue 2 - Typo in use of `resourceName` instead of `mergedResourceName` We did not actually experience this, but in the original code: ```java updateIdealState(helixManager, resourceName, anyIdealState -> finalUpdatedIdealState, retryPolicy, noChangeOk); ``` I believe `mergedResourceName` should actually be used instead on the off-chance that we picked up some outstanding update before processing ours. Luckily I think the risk of running into it is practically reduced by having 100 queues. -- 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]
