pkuwm commented on a change in pull request #1415:
URL: https://github.com/apache/helix/pull/1415#discussion_r495420417
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/constant/ZkSystemPropertyKeys.java
##########
@@ -36,6 +36,16 @@
public static final String ZK_SERIALIZER_ZNRECORD_AUTO_COMPRESS_ENABLED =
"zk.serializer.znrecord.auto-compress.enabled";
+ /**
+ * Setting this property to true enables ZNode bucketization, an operation
to divide a ZNRecord
+ * into specified buckets. This property applies to
+ * {@link org.apache.helix.zookeeper.datamodel.ZNRecordBucketizer}.
+ * <p>
+ * The default value is "true" (enabled).
+ */
+ public static final String ZK_BUCKETIZE_ZNRECORD_ENABLED =
+ "zk.bucketize.znrecord.enabled";
Review comment:
How about `zk.znrecord.bucketize.enabled` or
`zk.znrecord.bucketize.enabled`(which is consistent with the class
`ZNRecordBucketizer`)?
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -632,31 +636,33 @@ private void subscribeForChanges(NotificationContext.Type
callbackType, String p
case CUSTOMIZED_VIEW:
case TARGET_EXTERNAL_VIEW: {
// check if bucketized
- BaseDataAccessor<ZNRecord> baseAccessor = new
ZkBaseDataAccessor<>(_zkClient);
- List<ZNRecord> records = baseAccessor.getChildren(path, null, 0,
0, 0);
- for (ZNRecord record : records) {
- HelixProperty property = new HelixProperty(record);
- String childPath = path + "/" + record.getId();
-
- int bucketSize = property.getBucketSize();
- if (bucketSize > 0) {
- // subscribe both data-change and child-change on bucketized
parent node
- // data-change gives a delete-callback which is used to
remove watch
- List<String> bucketizedChildNames =
subscribeChildChange(childPath, callbackType);
+ if (BUCKETIZE_ZNRECORD_ENABLED) {
+ BaseDataAccessor<ZNRecord> baseAccessor = new
ZkBaseDataAccessor<>(_zkClient);
+ List<ZNRecord> records = baseAccessor.getChildren(path, null,
0, 0, 0);
+ for (ZNRecord record : records) {
+ HelixProperty property = new HelixProperty(record);
+ String childPath = path + "/" + record.getId();
+ int bucketSize = property.getBucketSize();
+ // Do data-change subscribe for both bucketized parent node
and non-bucketized node.
+ // For bucketized parent node, data-change gives a
delete-callback to remove watch.
subscribeDataChange(childPath, callbackType);
-
- // subscribe data-change on bucketized child
- if (bucketizedChildNames != null) {
- for (String bucketizedChildName : bucketizedChildNames) {
- String bucketizedChildPath = childPath + "/" +
bucketizedChildName;
- subscribeDataChange(bucketizedChildPath, callbackType);
+ if (bucketSize > 0) {
+ // subscribe child-change on bucketized parent node.
+ List<String> bucketizedChildNames =
+ subscribeChildChange(childPath, callbackType);
+
+ // subscribe data-change on bucketized child
+ if (bucketizedChildNames != null) {
+ for (String bucketizedChildName : bucketizedChildNames) {
+ String bucketizedChildPath = childPath + "/" +
bucketizedChildName;
+ subscribeDataChange(bucketizedChildPath, callbackType);
+ }
}
}
- } else {
- subscribeDataChange(childPath, callbackType);
}
+ break;
}
- break;
+ // go to default branch if bucketized feature is not enabled.
Review comment:
This is kind of tricky: readability is not very straightforward and it
is easy to make a mistake if later someone adds another case after this, it'll
break the logic.
How about this: we wrap the check into a private method and make it not easy
to make a mistake?
```
shouldReadBucketizedChildren(type) {
Set<> types; // CURRENT_STATE, ..., TARGET_EXTERNAL_VIEW
return types.contains(type) && BUCKETIZE_ZNRECORD_ENABLED;
}
// Then for the block
if (shouldReadBucketizedChildren(_changeType) {
// bucketized subscribe block
} else {
// default block
}
```
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixDataAccessor.java
##########
@@ -427,8 +437,9 @@ public boolean removeProperty(PropertyKey key) {
int bucketSize = property.getBucketSize();
if (bucketSize > 0) {
- // TODO: fix this if record.id != pathName
String childPath = parentPath + "/" + record.getId();
+ validateBucketizedEnabled(childPath, true /*isRead*/);
Review comment:
I guess we don't need a specific comment for a parameter like this in
Java? It is kind of impacting the code readability, what do you think? If you
like, I suggest you put it end of line or before line.
```
validateBucketizedEnabled(childPath, true); // true - is read
```
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixDataAccessor.java
##########
@@ -599,4 +610,12 @@ private int constructOptions(PropertyType type) {
List<DataUpdater<ZNRecord>> updaters, int options) {
return _baseDataAccessor.updateChildren(paths, updaters, options);
}
+
+ private void validateBucketizedEnabled(String path, boolean isRead) {
+ if (!BUCKETIZE_ZNRECORD_ENABLED) {
+ throw new HelixMetaDataAccessException(
+ "Can't " + (isRead ? "read" : "write") + " bucktized ZNode " + path
+ + " because Bucktize feature is not enabled. Please check ZK
system property 'zk.bucketize.znrecord.enabled' .");
Review comment:
Use `ZkSystemPropertyKeys.ZK_BUCKETIZE_ZNRECORD_ENABLED` instead of hard
coding it?
##########
File path:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixDataAccessor.java
##########
@@ -143,6 +147,7 @@ public boolean createMaintenance(MaintenanceSignal
maintenanceSignal) {
case EXTERNALVIEW:
// check if bucketized
if (value.getBucketSize() > 0) {
+ validateBucketizedEnabled(path, false /*isRead*/);
Review comment:
I see that there are so many places that we have to validate this. It
seems it is only using baseDataAccessor to read/write, can you help me
understand why we don't put this check inside baseDataAccessor?
And we don't check `Map<String, ZNRecord> map =
bucketizer.bucketize(lastCurState.getRecord());
` in `ParticipantManager`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
