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 2020/04/05 16:07:36 UTC
[GitHub] [zookeeper] severussundar opened a new pull request #1306:
ZOOKEEPER-3415: Convert internal logic to use streams
severussundar opened a new pull request #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306
As part of this PR, I have refactored some of the for loops with streams.
----------------------------------------------------------------
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 a change in pull request #1306:
ZOOKEEPER-3415: Convert internal logic to use streams
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306#discussion_r403873499
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ContainerManager.java
##########
@@ -125,24 +125,29 @@ public void stop() {
*/
public void checkContainers() throws InterruptedException {
long minIntervalMs = getMinIntervalMs();
- for (String containerPath : getCandidates()) {
- long startMs = Time.currentElapsedTime();
-
- ByteBuffer path = ByteBuffer.wrap(containerPath.getBytes());
- Request request = new Request(null, 0, 0, ZooDefs.OpCode.deleteContainer, path, null);
- try {
- LOG.info("Attempting to delete candidate container: {}", containerPath);
- postDeleteRequest(request);
- } catch (Exception e) {
- LOG.error("Could not delete container: {}", containerPath, e);
- }
+ getCandidates().stream()
+ .forEach(containerPath -> {
+ long startMs = Time.currentElapsedTime();
- long elapsedMs = Time.currentElapsedTime() - startMs;
- long waitMs = minIntervalMs - elapsedMs;
- if (waitMs > 0) {
- Thread.sleep(waitMs);
- }
- }
+ ByteBuffer path = ByteBuffer.wrap(containerPath.getBytes());
+ Request request = new Request(null, 0, 0, ZooDefs.OpCode.deleteContainer, path, null);
+ try {
+ LOG.info("Attempting to delete candidate container: {}", containerPath);
+ postDeleteRequest(request);
+ } catch (Exception e) {
+ LOG.error("Could not delete container: {}", containerPath, e);
+ }
+
+ long elapsedMs = Time.currentElapsedTime() - startMs;
+ long waitMs = minIntervalMs - elapsedMs;
+ if (waitMs > 0) {
+ try {
+ Thread.sleep(waitMs);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+ }
+ });
}
Review comment:
this change makes the body of `forEach` so fat? just a personal aesthetics.
----------------------------------------------------------------
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] severussundar commented on a change in pull request
#1306: ZOOKEEPER-3415: Convert internal logic to use streams
Posted by GitBox <gi...@apache.org>.
severussundar commented on a change in pull request #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306#discussion_r404587971
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ContainerManager.java
##########
@@ -125,24 +125,29 @@ public void stop() {
*/
public void checkContainers() throws InterruptedException {
long minIntervalMs = getMinIntervalMs();
- for (String containerPath : getCandidates()) {
- long startMs = Time.currentElapsedTime();
-
- ByteBuffer path = ByteBuffer.wrap(containerPath.getBytes());
- Request request = new Request(null, 0, 0, ZooDefs.OpCode.deleteContainer, path, null);
- try {
- LOG.info("Attempting to delete candidate container: {}", containerPath);
- postDeleteRequest(request);
- } catch (Exception e) {
- LOG.error("Could not delete container: {}", containerPath, e);
- }
+ getCandidates().stream()
+ .forEach(containerPath -> {
+ long startMs = Time.currentElapsedTime();
- long elapsedMs = Time.currentElapsedTime() - startMs;
- long waitMs = minIntervalMs - elapsedMs;
- if (waitMs > 0) {
- Thread.sleep(waitMs);
- }
- }
+ ByteBuffer path = ByteBuffer.wrap(containerPath.getBytes());
+ Request request = new Request(null, 0, 0, ZooDefs.OpCode.deleteContainer, path, null);
+ try {
+ LOG.info("Attempting to delete candidate container: {}", containerPath);
+ postDeleteRequest(request);
+ } catch (Exception e) {
+ LOG.error("Could not delete container: {}", containerPath, e);
+ }
+
+ long elapsedMs = Time.currentElapsedTime() - startMs;
+ long waitMs = minIntervalMs - elapsedMs;
+ if (waitMs > 0) {
+ try {
+ Thread.sleep(waitMs);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+ }
+ });
}
Review comment:
Thanks for the comment! I'll revert back to the original for loop
----------------------------------------------------------------
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 a change in pull request #1306:
ZOOKEEPER-3415: Convert internal logic to use streams
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306#discussion_r403867869
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
##########
@@ -241,13 +240,14 @@ public int getEphemeralsCount() {
*/
public long approximateDataSize() {
long result = 0;
- for (Map.Entry<String, DataNode> entry : nodes.entrySet()) {
- DataNode value = entry.getValue();
- synchronized (value) {
- result += getNodeSize(entry.getKey(), value.data);
- }
- }
- return result;
+
+ for (Map.Entry<String, DataNode> entry : nodes.entrySet()) {
+ DataNode value =entry.getValue();
+ synchronized (value) {
+ result +=getNodeSize(entry.getKey(), value.data);
+ }
+ }
+ return result;
}
Review comment:
why need this?
----------------------------------------------------------------
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] severussundar commented on issue #1306: ZOOKEEPER-3415:
Convert internal logic to use streams
Posted by GitBox <gi...@apache.org>.
severussundar commented on issue #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306#issuecomment-610467429
@eolivelli, thank you for your valuable feedback. I'll rework and raise a new PR targeting master. I'll decline this PR.
----------------------------------------------------------------
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 #1306:
ZOOKEEPER-3415: Convert internal logic to use streams
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306#discussion_r404605791
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
##########
@@ -227,11 +230,7 @@ public int getWatchCount() {
}
public int getEphemeralsCount() {
- int result = 0;
- for (HashSet<String> set : ephemerals.values()) {
- result += set.size();
- }
- return result;
+ return ephemerals.values().stream().mapToInt(i -> i.size()).sum();
Review comment:
what's the value of this change ?
----------------------------------------------------------------
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 #1306:
ZOOKEEPER-3415: Convert internal logic to use streams
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306#discussion_r404605921
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
##########
@@ -1175,30 +1174,33 @@ void killSession(long session, long zxid, Set<String> paths2DeleteLocal,
}
void deleteNodes(long session, long zxid, Iterable<String> paths2Delete) {
- for (String path : paths2Delete) {
- boolean deleted = false;
- String sessionHex = "0x" + Long.toHexString(session);
- try {
- deleteNode(path, zxid);
- deleted = true;
- LOG.debug("Deleting ephemeral node {} for session {}", path, sessionHex);
- } catch (NoNodeException e) {
- LOG.warn(
- "Ignoring NoNodeException for path {} while removing ephemeral for dead session {}",
- path, sessionHex);
- }
- if (ZKAuditProvider.isAuditEnabled()) {
- if (deleted) {
- ZKAuditProvider.log(ZKAuditProvider.getZKUser(),
- AuditConstants.OP_DEL_EZNODE_EXP, path, null, null,
- sessionHex, null, Result.SUCCESS);
- } else {
- ZKAuditProvider.log(ZKAuditProvider.getZKUser(),
- AuditConstants.OP_DEL_EZNODE_EXP, path, null, null,
- sessionHex, null, Result.FAILURE);
+
+ StreamSupport.stream(paths2Delete.spliterator(),false)
Review comment:
what's the benefit of this change ?
----------------------------------------------------------------
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] severussundar closed pull request #1306:
ZOOKEEPER-3415: Convert internal logic to use streams
Posted by GitBox <gi...@apache.org>.
severussundar closed pull request #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306
----------------------------------------------------------------
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 a change in pull request #1306:
ZOOKEEPER-3415: Convert internal logic to use streams
Posted by GitBox <gi...@apache.org>.
maoling commented on a change in pull request #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306#discussion_r403870084
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
##########
@@ -1625,12 +1618,9 @@ private static boolean shouldRequireClientSaslAuth() {
}
private boolean hasCnxSASLAuthenticated(ServerCnxn cnxn) {
- for (Id id : cnxn.getAuthInfo()) {
- if (id.getScheme().equals(SASL_AUTH_SCHEME)) {
- return true;
- }
- }
- return false;
+
+ return cnxn.getAuthInfo().stream().filter(id -> id.getScheme().equals(SASL_AUTH_SCHEME)).count() > 0;
+
}
Review comment:
` return cnxn.getAuthInfo().stream().anyMatch(id -> id.getScheme().equals(SASL_AUTH_SCHEME));` is better?
----------------------------------------------------------------
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] severussundar commented on a change in pull request
#1306: ZOOKEEPER-3415: Convert internal logic to use streams
Posted by GitBox <gi...@apache.org>.
severussundar commented on a change in pull request #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306#discussion_r404564618
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
##########
@@ -1625,12 +1618,9 @@ private static boolean shouldRequireClientSaslAuth() {
}
private boolean hasCnxSASLAuthenticated(ServerCnxn cnxn) {
- for (Id id : cnxn.getAuthInfo()) {
- if (id.getScheme().equals(SASL_AUTH_SCHEME)) {
- return true;
- }
- }
- return false;
+
+ return cnxn.getAuthInfo().stream().filter(id -> id.getScheme().equals(SASL_AUTH_SCHEME)).count() > 0;
+
}
Review comment:
HI maoling, good suggestion. I'll update it. Thanks!
----------------------------------------------------------------
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 issue #1306: ZOOKEEPER-3415:
Convert internal logic to use streams
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306#issuecomment-610233249
I just noticed that you are targeting branch-3.6.
you should target "master"
----------------------------------------------------------------
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] severussundar commented on a change in pull request
#1306: ZOOKEEPER-3415: Convert internal logic to use streams
Posted by GitBox <gi...@apache.org>.
severussundar commented on a change in pull request #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306#discussion_r404595665
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
##########
@@ -241,13 +240,14 @@ public int getEphemeralsCount() {
*/
public long approximateDataSize() {
long result = 0;
- for (Map.Entry<String, DataNode> entry : nodes.entrySet()) {
- DataNode value = entry.getValue();
- synchronized (value) {
- result += getNodeSize(entry.getKey(), value.data);
- }
- }
- return result;
+
+ for (Map.Entry<String, DataNode> entry : nodes.entrySet()) {
+ DataNode value =entry.getValue();
+ synchronized (value) {
+ result +=getNodeSize(entry.getKey(), value.data);
+ }
+ }
+ return result;
}
Review comment:
I was trying to use a stream here but couldn't come up with the correct approach and reverted back to the original for loop. I'll remove the extra indendations.
----------------------------------------------------------------
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 #1306:
ZOOKEEPER-3415: Convert internal logic to use streams
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306#discussion_r403723175
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/JLineZNodeCompleter.java
##########
@@ -65,11 +67,8 @@ private int completeZNode(String buffer, String token, List<String> candidates)
// Only the root path can end in a /, so strip it off every other prefix
String dir = idx == 1 ? "/" : path.substring(0, idx - 1);
List<String> children = zk.getChildren(dir, false);
- for (String child : children) {
- if (child.startsWith(prefix)) {
- candidates.add(child);
- }
- }
+ candidates = children.stream().filter(child -> child.startsWith(prefix)).collect(Collectors.toList());
Review comment:
Personally I don't find a real benefit and code actually doesn't look better.
Why do you want to change this loop?
It is actually using more resources than the fore each loop.
This is not an hot path but the question is more general.
Streams look appealing but sometimes the overhead is not worth
----------------------------------------------------------------
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 #1306:
ZOOKEEPER-3415: Convert internal logic to use streams
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1306: ZOOKEEPER-3415: Convert internal logic to use streams
URL: https://github.com/apache/zookeeper/pull/1306#discussion_r403723270
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
##########
@@ -758,9 +758,8 @@ public Stat statNode(String path, Watcher watcher) throws KeeperException.NoNode
}
int bytes = 0;
- for (String child : children) {
- bytes += child.length();
- }
+ bytes = children.stream().mapToInt(child -> child.length()).sum();
Review comment:
This is not better than the original code
----------------------------------------------------------------
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