Re: [PR] improves ZooCache node tracking [accumulo]

["kevinrr888 (via GitHub)" <gi...@apache.org>]

kevinrr888 commented on code in PR #5286:
URL: https://github.com/apache/accumulo/pull/5286#discussion_r1941910798


##########
core/src/main/java/org/apache/accumulo/core/zookeeper/ZcNode.java:
##########
@@ -29,27 +29,42 @@
  * and children are obtained from zookeeper at different times. This class is structured so that
  * data can be obtained first and then children added later or visa veras.
  *
- * <p>
- * Four distinct states can be cached for a zookeeper node.
- * <ul>
- * <li>Can cache that a node does not exist in zookeeper. This state is represented by data, state,
- * and children all being null.</li>
- * <li>Can cache only the data for a zookeeper node. For this state data and stat are non-null while
- * children is null. Calling getChildren on node in this state will throw an exception.</li>
- * <li>Can cache only the children for a zookeeper node. For this state children is non-null while
- * data and stat are null. Calling getData or getStat on node in this state will throw an
- * exception.</li>
- * <li>Can cache the children and data for a zookeeper node. For this state data,stat, and children
- * are non-null.</li>
- * </ul>
- * <p>
- *
  */
 class ZcNode {
 
+  /**
+   * This enum represents what ZooCache has discovered about a given node in zookeeper so far.
+   */
+  enum Discovered {
+    /**
+     * An attempt was made to fetch data or children and ZooKeeper threw a NoNodeException. In this
+     * case ZooCache knows the node did not exist.
+     */
+    NON_EXISTENT,
+    /**
+     * ZooCache knows the node existed and what its children were because a successful call was made
+     * to ZooKepper to get children. However zoocache has never requested the nodes data and does

Review Comment:
   ```suggestion
        * to ZooKeeper to get children. However zoocache has never requested the nodes data and does
   ```



##########
core/src/main/java/org/apache/accumulo/core/zookeeper/ZcNode.java:
##########
@@ -29,27 +29,42 @@
  * and children are obtained from zookeeper at different times. This class is structured so that
  * data can be obtained first and then children added later or visa veras.
  *
- * <p>
- * Four distinct states can be cached for a zookeeper node.
- * <ul>
- * <li>Can cache that a node does not exist in zookeeper. This state is represented by data, state,
- * and children all being null.</li>
- * <li>Can cache only the data for a zookeeper node. For this state data and stat are non-null while
- * children is null. Calling getChildren on node in this state will throw an exception.</li>
- * <li>Can cache only the children for a zookeeper node. For this state children is non-null while
- * data and stat are null. Calling getData or getStat on node in this state will throw an
- * exception.</li>

Review Comment:
   Could keep the comments regarding what method calls in what state will result in an exception



##########
core/src/main/java/org/apache/accumulo/core/zookeeper/ZcNode.java:
##########
@@ -29,27 +29,42 @@
  * and children are obtained from zookeeper at different times. This class is structured so that
  * data can be obtained first and then children added later or visa veras.
  *
- * <p>
- * Four distinct states can be cached for a zookeeper node.
- * <ul>
- * <li>Can cache that a node does not exist in zookeeper. This state is represented by data, state,
- * and children all being null.</li>
- * <li>Can cache only the data for a zookeeper node. For this state data and stat are non-null while
- * children is null. Calling getChildren on node in this state will throw an exception.</li>
- * <li>Can cache only the children for a zookeeper node. For this state children is non-null while
- * data and stat are null. Calling getData or getStat on node in this state will throw an
- * exception.</li>
- * <li>Can cache the children and data for a zookeeper node. For this state data,stat, and children
- * are non-null.</li>
- * </ul>
- * <p>
- *
  */
 class ZcNode {
 
+  /**
+   * This enum represents what ZooCache has discovered about a given node in zookeeper so far.
+   */
+  enum Discovered {
+    /**
+     * An attempt was made to fetch data or children and ZooKeeper threw a NoNodeException. In this
+     * case ZooCache knows the node did not exist.
+     */
+    NON_EXISTENT,
+    /**
+     * ZooCache knows the node existed and what its children were because a successful call was made
+     * to ZooKepper to get children. However zoocache has never requested the nodes data and does
+     * not know its data.
+     */
+    CHILDREN_ONLY,
+    /**
+     * ZooCache knows the node exist and what its data was because a successful call was mde to

Review Comment:
   ```suggestion
        * ZooCache knows the node exists and what its data was because a successful call was made to
   ```



##########
core/src/main/java/org/apache/accumulo/core/zookeeper/ZcNode.java:
##########
@@ -83,13 +116,29 @@ private ZcNode() {
    * stat.
    */
   ZcNode(byte[] data, ZcStat zstat, ZcNode existing) {
-    this.data = Objects.requireNonNull(data);
-    this.stat = Objects.requireNonNull(zstat);

Review Comment:
   Does not make a difference so feel free to ignore: I think it would be a bit clearer to keep these checks at the top of the constructor instead of at the end.



-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org