-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new key generator "LeafSegmentKeyGenerator" and related functions. #2449
Conversation
@terrymanu . |
You can try to use curator-test which can startup a zookeeper in memory, or use mock to do unit-test |
@terrymanu .THX, I will try it later. |
Nice work, thank you. |
@wgy8283335 Thanks, i think it is great after a quick look. |
@tristaZero. Thank you. |
After reading your pr carefully and digging into the Leaf-segement and Leaf-Snowflake, i give my opinions about this pr, and also want to have 1.The package of sharding-core-common should be basic package which is used by other packages, so it is better to move all the classes related to LeafSegmentKeyGenerator to sharding-orchestration-core. 2.When LeafSegmentKeyGenerator is in sharding-orchestration-core, maybe we can consider to use CuratorZookeeperRegistryCenter instead of LeafCuratorZookeeper. The function of incrementCacheId in LeafCuratorZookeeper can considered to move to LeafSegmentKeyGenerator, and moving tryLock() to CuratorZookeeperRegistryCenter is ok? 3.The API of generateKey() now is to return null in LeafSegmentKeyGenerator, but it is not allowed to modify APIs. Therefore using generateKey(), not generateKey(tableName) is a better choice. 4.Actually, LeafSegmentKeyGenerator really needs tableName as a key in zk. So i suggest set tableName in properties of LeafSegmentKeyGenerator and getTableName() from properties to use it in zk. 5.The another important thing is multiple schemas. If user use same tableName in multiple schemas, the node value of same table name in zk will be overwrite. So please consider use schemaName and tableName as note key. |
Fixes #1775.
Changes proposed in this pull request: