ZOOKEEPER-3020: Review of SyncRequestProcessor #876

belugabehr · 2019-03-28T02:32:37Z

Use ArrayDeque instead of LinkedList
Use ThreadLocalRandom instead of Random
Remove the 'running' flag - use the Thread#join facility to detect if the thread has stopped running. Using a flag can cause race condition issues and is superfluous.
Make static final variable names in all caps
General cleanup

This class is likely to be faster than Stack when used as a stack, and faster than LinkedList when used as a queue.

https://docs.oracle.com/javase/7/docs/api/java/util/ArrayDeque.html

belugabehr · 2019-03-28T12:46:18Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/SyncRequestProcessor.java

                if (si == requestOfDeath) {
                    break;
                }
-                if (si != null) {


This looks like a lot of code change, but I simply removed this check 'is null' check and subsequently had to fix the indentation of the large block of code beneath it.

jhuan31 · 2019-03-28T15:59:12Z

I'm wondering why findbugs didn't complain about the unused code... I filed PR #853 because findbugs complained in PR #850, which seems pretty innocent--just add a few metric lines. I guess either this PR or #850 changed the original logic in a subtle way.

anmolnar

+1
Great refactoring.

anmolnar · 2019-04-02T12:12:54Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/SyncRequestProcessor.java

+            this.join();
+            this.flush();
+        } catch (InterruptedException e) {
            LOG.warn("Interrupted while wating for " + this + " to finish");


Nit: LOG.warn("Interrupted while wating for {} to finish", this);

@anmolnar Well, if you can believe it, lol, I am trying to touch less where I can in order to make a review easier. Also, using SLF4J parameter-ized logging is the fastest way to do logging when logging is disabled. It would be very unlikely that a WARN message is disabled so it's actually faster to just keep it as is. If I was writing this code from scratch, I would use a parameter logging message here for consistency, but there's no strong argument to change it now.

Got you. You can leave it as is.
In my understanding parameterised logging is not slower than constructing message beforehand, but I agree that it only has performance gain when not logging the message.

anmolnar · 2019-04-02T12:14:28Z

@belugabehr Please update the comment and description of this PR. Thanks!

belugabehr · 2019-04-02T12:42:17Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/SyncRequestProcessor.java

    private final RequestProcessor nextProcessor;

-    private Thread snapInProcess = null;
+    private final Semaphore snapThreadMutex = new Semaphore(1);


So, just FYI. I wanted to make this change because of two reasons:

It removes the special-case 'null' check (line 131) when the first iteration occurs.

The last 'snapInProcess' variable is never cleared out so there is always a dangling pointer to the last instance that ran... i.e., it never gets GC'ed until the next time a snap process runs and kicks the previous reference out.

anmolnar · 2019-04-03T10:44:36Z

@belugabehr Jira doesn't indicate that, but I suppose this is going to be a master-only commit

belugabehr · 2019-04-03T12:53:37Z

@anmolnar Yes. All of my work is for master, unless anyone has strong feelings that it should be committed elsewhere. That decision is above my pay grade here :)

enixon

Thanks @belugabehr , for breaking this in to pieces! Much easier to review. :)

Overall looks very sane to me, just a couple of points.

enixon · 2019-04-08T23:42:03Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/SyncRequestProcessor.java

+                    continue;
+                }
+                toFlush.add(si);
+                if (toFlush.size() == FLUSH_SIZE) {


any danger from changing > to == here?

The only things that adds an item to this collection is the thread itself, and the 'add' exists immediately before the check against FLUSH_SIZE. There is no risk of somehow jumping over the threshold. A 'greater than' check is confusing to the reader because it implies that there might be some way to jump the threshold.

enixon · 2019-04-08T23:56:25Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/SyncRequestProcessor.java

            while (true) {
-                Request si = null;
-                if (toFlush.isEmpty()) {
+                Request si = queuedRequests.poll();


Is behavior changing here? Your logic of tolling from a poll directly to a take instead of forcing a loop via continue looks right to me - is there a way that take can respond with a null value or otherwise break something in the new arrangement?

Retrieves and removes the head of this queue, waiting if necessary until an element becomes available.

https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/BlockingQueue.html#take()

The only way a 'null' could become available here is if a 'null' value is inserted into the collection. However, that is prevented on line 212.

I believe this is a good improvement because previously it was always checking 'isEmpty' to make a decision about how to interact with the 'queuedRequests' queue. I have removed that condition and it will just go ahead and try to pull something from the queue.

enixon · 2019-04-09T05:22:44Z

lgtm +1 (non-committer)

belugabehr · 2019-04-13T15:07:31Z

@anmolnar Any additional ideas for improvements?

anmolnar · 2019-04-30T16:08:49Z

Committed to master branch. Thanks @belugabehr !

1. Use ArrayDeque instead of LinkedList 2. Use ThreadLocalRandom instead of Random 3. Remove the 'running' flag - use the Thread#join facility to detect if the thread has stopped running. Using a flag can cause race condition issues and is superfluous. 4. Make static final variable names in all caps 5. General cleanup > This class is likely to be faster than Stack when used as a stack, and faster than LinkedList when used as a queue. https://docs.oracle.com/javase/7/docs/api/java/util/ArrayDeque.html Author: Beluga Behr <dam6923@gmail.com> Reviewers: andor@apache.org Closes apache#876 from BELUGABEHR/ZOOKEEPER-3020-2 and squashes the following commits: 8f1df37 [Beluga Behr] General cleanup 83cab73 [Beluga Behr] Simplify and streamline access pattern of queuedRequests 5e0b801 [Beluga Behr] Use a semaphore to track status of snapshot thread d119ef7 [Beluga Behr] No need to keep a flag which states if the thread is alive e2059cf [Beluga Behr] Replace LinkedList with ArrayDeque 57a0449 [Beluga Behr] Replace Random with ThreadLocalRandom 17bd1fa [Beluga Behr] Require that the request not be 'null' b28ff94 [Beluga Behr] Refactor flush method

belugabehr added 8 commits March 27, 2019 22:07

Refactor flush method

b28ff94

Require that the request not be 'null'

17bd1fa

Replace Random with ThreadLocalRandom

57a0449

Replace LinkedList with ArrayDeque

e2059cf

No need to keep a flag which states if the thread is alive

d119ef7

Use a semaphore to track status of snapshot thread

5e0b801

Simplify and streamline access pattern of queuedRequests

83cab73

General cleanup

8f1df37

belugabehr commented Mar 28, 2019

View reviewed changes

belugabehr mentioned this pull request Mar 28, 2019

ZOOKEEPER-3020: Review of SyncRequestProcessor #869

Closed

anmolnar approved these changes Apr 2, 2019

View reviewed changes

belugabehr changed the title ~~Zookeeper 3020 2~~ ZOOKEEPER-3020: Review of SyncRequestProcessor Apr 2, 2019

belugabehr commented Apr 2, 2019

View reviewed changes

enixon reviewed Apr 8, 2019

View reviewed changes

asfgit closed this in 876aaf4 Apr 30, 2019

enixon mentioned this pull request May 7, 2019

ZOOKEEPER-3311: Allow a delay to the transaction log flush #851

Closed

ZOOKEEPER-3020: Review of SyncRequestProcessor #876

ZOOKEEPER-3020: Review of SyncRequestProcessor #876

Uh oh!

Conversation

belugabehr commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhuan31 commented Mar 28, 2019

Uh oh!

anmolnar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmolnar commented Apr 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmolnar commented Apr 3, 2019

Uh oh!

belugabehr commented Apr 3, 2019

Uh oh!

enixon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enixon commented Apr 9, 2019

Uh oh!

belugabehr commented Apr 13, 2019

Uh oh!

anmolnar commented Apr 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

belugabehr commented Mar 28, 2019 •

edited

Loading