[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1311: Zookeeper 3415

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #1311:
URL: https://github.com/apache/zookeeper/pull/1311#discussion_r455501578



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
##########
@@ -488,17 +488,9 @@ public void loadData() throws IOException, InterruptedException {
         }
 
         // Clean up dead sessions
-        List<Long> deadSessions = new ArrayList<>();
-        for (Long session : zkDb.getSessions()) {
-            if (zkDb.getSessionWithTimeOuts().get(session) == null) {
-                deadSessions.add(session);
-            }
-        }
-
-        for (long session : deadSessions) {
-            // TODO: Is lastProcessedZxid really the best thing to use?
-            killSession(session, zkDb.getDataTreeLastProcessedZxid());
-        }
+        zkDb.getSessions().stream()

Review comment:
       @eolivelli Streams don't add much overhead, when used properly. They might add some overhead sometimes, but that's not always the case, and even when they do add a slight overhead, the increased reliability of the code, due to its readability/maintainability can be worth the trade.
   
   In this case, it:
   
   1. Makes two loops (`O(n + m)`) into a single loop (`O(n)`),
   2. Avoids creating an intermediate data structure entirely (saving 'm' memory units), and
   3. It's much nicer to read.
   
   I see no reason to not apply these changes.




----------------------------------------------------------------
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