[Issue 6068] Fixing the calls to Helix to throw exception if zk conne… #6069
Conversation
…ction is broken See Issue apache#6068
These APIs will ensure that if there is a zk disconnect we will get an exception after a minimal number of retries. We can change this to retry once and implement a backoff retry if needed later on. Note that the underlying helix library ends up calling the previous API (as yet), but we will upgrade to a helix version soon that actually implements these
|
@Jackie-Jiang I made one more change. Can you take another look please? thanks |
| @@ -305,7 +308,7 @@ public static Schema getTableSchema(@Nonnull ZkHelixPropertyStore<ZNRecord> prop | |||
| ZkHelixPropertyStore<ZNRecord> propertyStore, String tableName) { | |||
| String offlineTableName = TableNameBuilder.OFFLINE.tableNameWithType(tableName); | |||
| String parentPath = constructPropertyStorePathForResource(offlineTableName); | |||
| List<ZNRecord> znRecords = propertyStore.getChildren(parentPath, null, AccessOption.PERSISTENT); | |||
| List<ZNRecord> znRecords = propertyStore.getChildren(parentPath, null, AccessOption.PERSISTENT, ZK_OP_RETRY_COUNT, ZK_OP_RETRY_INTERVAL_MS); | |||
There was a problem hiding this comment.
This won't take effect... Here is the Helix code for it (I would count it as a bug):
@Override
public List<T> getChildren(String parentPath, List<Stat> stats, int options, int retryCount,
int retryInterval) throws HelixException {
return getChildren(parentPath, stats, options);
}
I don't see an easy way to throw exception for this API unless we create the children paths manually.
There was a problem hiding this comment.
We are working with the helix team to give us a new version with the API implemented. Since I took the trouble to trace all the APIs, I took the liberty to modify them so that when we upgrade to newer helix version, we have the right calls
|
@mcvsubbu We want both |
Since the new API has not been implemented yet, there will not be any unexpected behavior at this time, right? |
|
@mcvsubbu The unexpected behavior I was referring to is that we explicitly set retry in the method, but Helix won't do the retry, and won't throw exception when the read fails. So I suggest not doing the explicit retry before upgrading the Helix version. But this is not critical. Feel free to merge |
…ction is broken
See Issue #6068
Description
Add a description of your PR here.
A good description should include pointers to an issue or design document, etc.
Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
backward-incompat, and complete the section below on Release Notes)Does this PR fix a zero-downtime upgrade introduced earlier?
backward-incompat, and complete the section below on Release Notes)Does this PR otherwise need attention when creating release notes? Things to consider:
release-notesand complete the section on Release Notes)Release Notes
If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release.
If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text
Documentation
If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document