You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2019/12/23 19:56:52 UTC
[GitHub] [zookeeper] belugabehr opened a new pull request #1193:
ZOOKEEPER-3663: Clean Up ZNodeName Class
belugabehr opened a new pull request #1193: ZOOKEEPER-3663: Clean Up ZNodeName Class
URL: https://github.com/apache/zookeeper/pull/1193
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] asfgit closed pull request #1193: ZOOKEEPER-3663: Clean
Up ZNodeName Class
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1193: ZOOKEEPER-3663: Clean Up ZNodeName Class
URL: https://github.com/apache/zookeeper/pull/1193
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] belugabehr commented on a change in pull request #1193:
ZOOKEEPER-3663: Clean Up ZNodeName Class
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1193: ZOOKEEPER-3663: Clean Up ZNodeName Class
URL: https://github.com/apache/zookeeper/pull/1193#discussion_r361213861
##########
File path: zookeeper-recipes/zookeeper-recipes-lock/src/main/java/org/apache/zookeeper/recipes/lock/ZNodeName.java
##########
@@ -18,45 +18,62 @@
package org.apache.zookeeper.recipes.lock;
+import java.util.Objects;
+import java.util.Optional;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Represents an ephemeral znode name which has an ordered sequence number
- * and can be sorted in order.
+ * Represents an immutable ephemeral znode name which has an ordered sequence
+ * number and can be sorted in order. The expected name format of the znode is
+ * as follows:
*
+ * <pre>
+ * <name>-<sequence>
+ *
+ * For example: lock-00001
+ * </pre>
*/
class ZNodeName implements Comparable<ZNodeName> {
- private final String name;
- private String prefix;
- private int sequence = -1;
private static final Logger LOG = LoggerFactory.getLogger(ZNodeName.class);
- public ZNodeName(String name) {
- if (name == null) {
- throw new NullPointerException("id cannot be null");
- }
- this.name = name;
- this.prefix = name;
- int idx = name.lastIndexOf('-');
- if (idx >= 0) {
+ private final String name;
+ private final String prefix;
+ private final Optional<Integer> sequence;
Review comment:
@eolivelli I always prefer using Optional. It's always very clear as to its usage and the caller must deal with it. Better than forcing the user to correctly check for a special negative value that indicates 'no sequence available'. This is not performance-critical code, so readability and usability should be prioritized.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] belugabehr commented on issue #1193: ZOOKEEPER-3663:
Clean Up ZNodeName Class
Posted by GitBox <gi...@apache.org>.
belugabehr commented on issue #1193: ZOOKEEPER-3663: Clean Up ZNodeName Class
URL: https://github.com/apache/zookeeper/pull/1193#issuecomment-568791708
@eolivelli Thank you always for your reviews.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] belugabehr commented on a change in pull request #1193:
ZOOKEEPER-3663: Clean Up ZNodeName Class
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1193: ZOOKEEPER-3663: Clean Up ZNodeName Class
URL: https://github.com/apache/zookeeper/pull/1193#discussion_r361213999
##########
File path: zookeeper-recipes/zookeeper-recipes-lock/src/main/java/org/apache/zookeeper/recipes/lock/ZNodeName.java
##########
@@ -102,9 +125,9 @@ public String getName() {
}
/**
- * Returns the sequence number.
+ * Returns the optional sequence number.
*/
- public int getZNodeName() {
+ public Optional<Integer> getSequence() {
Review comment:
All of the other `getters` are also public, so I think it should be consistent with them.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1193:
ZOOKEEPER-3663: Clean Up ZNodeName Class
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1193: ZOOKEEPER-3663: Clean Up ZNodeName Class
URL: https://github.com/apache/zookeeper/pull/1193#discussion_r361194176
##########
File path: zookeeper-recipes/zookeeper-recipes-lock/src/main/java/org/apache/zookeeper/recipes/lock/ZNodeName.java
##########
@@ -18,45 +18,62 @@
package org.apache.zookeeper.recipes.lock;
+import java.util.Objects;
+import java.util.Optional;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
- * Represents an ephemeral znode name which has an ordered sequence number
- * and can be sorted in order.
+ * Represents an immutable ephemeral znode name which has an ordered sequence
+ * number and can be sorted in order. The expected name format of the znode is
+ * as follows:
*
+ * <pre>
+ * <name>-<sequence>
+ *
+ * For example: lock-00001
+ * </pre>
*/
class ZNodeName implements Comparable<ZNodeName> {
- private final String name;
- private String prefix;
- private int sequence = -1;
private static final Logger LOG = LoggerFactory.getLogger(ZNodeName.class);
- public ZNodeName(String name) {
- if (name == null) {
- throw new NullPointerException("id cannot be null");
- }
- this.name = name;
- this.prefix = name;
- int idx = name.lastIndexOf('-');
- if (idx >= 0) {
+ private final String name;
+ private final String prefix;
+ private final Optional<Integer> sequence;
Review comment:
The difference from int to Optional<Integer> in terms of space/memory usage is considerable.
Is it really worth to change it ?
This is only a 'recipe' so not a big deal, but I wonder if is it better to keep it simpler.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] maoling commented on issue #1193: ZOOKEEPER-3663: Clean
Up ZNodeName Class
Posted by GitBox <gi...@apache.org>.
maoling commented on issue #1193: ZOOKEEPER-3663: Clean Up ZNodeName Class
URL: https://github.com/apache/zookeeper/pull/1193#issuecomment-568825825
@belugabehr
You may also interested in [ZOOKEEPER-3221](https://issues.apache.org/jira/browse/ZOOKEEPER-3221). Haha. I am issue recommender system.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] anmolnar commented on issue #1193: ZOOKEEPER-3663:
Clean Up ZNodeName Class
Posted by GitBox <gi...@apache.org>.
anmolnar commented on issue #1193: ZOOKEEPER-3663: Clean Up ZNodeName Class
URL: https://github.com/apache/zookeeper/pull/1193#issuecomment-576784875
Committed to master branch. Thanks @belugabehr !
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1193:
ZOOKEEPER-3663: Clean Up ZNodeName Class
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1193: ZOOKEEPER-3663: Clean Up ZNodeName Class
URL: https://github.com/apache/zookeeper/pull/1193#discussion_r361193972
##########
File path: zookeeper-recipes/zookeeper-recipes-lock/src/main/java/org/apache/zookeeper/recipes/lock/ZNodeName.java
##########
@@ -102,9 +125,9 @@ public String getName() {
}
/**
- * Returns the sequence number.
+ * Returns the optional sequence number.
*/
- public int getZNodeName() {
+ public Optional<Integer> getSequence() {
Review comment:
it looks like this method is called only in tests.
you can drop the 'public' modifier
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services