ZOOKEEPER-2694:sync CLI command does not wait for result from server #823

maoling · 2019-02-19T03:24:01Z

Thanks the original work from arshad.mohammad
more details in ZOOKEEPER-2694

anmolnar

Overall the fix looks good to me, just having a few questions.

anmolnar · 2019-03-12T18:22:05Z

zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java

                }
            }, null);
-        } catch (IllegalArgumentException ex) {
+            latch.await();


Wouldn't it be safer to wait for some reasonable amount of time (30s) rather than waiting indefinitely?

Maybe it can be a parameter with a default of 30secs

anmolnar · 2019-03-12T18:22:47Z

zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java

-        } catch (IllegalArgumentException ex) {
+            latch.await();
+            out.println("Sync returned " + resultCode[0]);
+        } catch (IllegalArgumentException | InterruptedException ex) {


Do you need to catch InterruptedException here?

latch.await needs this, my sweet IDEA help me do it.

I don't think it's handled properly here. You basically swallowing and hiding it inside a MalformedPathException which is wrong.

eolivelli · 2019-03-12T20:13:10Z

zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java

                }
            }, null);
-        } catch (IllegalArgumentException ex) {
+            latch.await();


Maybe it can be a parameter with a default of 30secs

eolivelli · 2019-03-12T20:14:43Z

zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java

@@ -55,14 +57,18 @@ public CliCommand parse(String[] cmdArgs) throws CliParseException {
    @Override
    public boolean exec() throws CliException {
        String path = args[1];
+        CountDownLatch latch = new CountDownLatch(1);


What about a more modern CompletableFuture?

Thanks for mentioning this good option:CompletableFuture,but for issue,IMO,we don't need to kill a chicken with the sword：D

CompletableFuture has specifically been designed for scenarios like this. As far as I'm concerned. Let me check.

How about this?

@Override public boolean exec() throws CliException { String path = args[1]; CompletableFuture<Integer> cf = new CompletableFuture<>(); try { zk.sync(path, new AsyncCallback.VoidCallback() { public void processResult(int rc, String path, Object ctx) { cf.complete(rc); } }, null); try { int resultCode = cf.get(CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS); out.println("Sync returned " + resultCode); } catch (TimeoutException ex) { out.println("Sync is timeout within " + CONNECTION_TIMEOUT + " ms"); } } catch (IllegalArgumentException | InterruptedException | ExecutionException ex) { throw new MalformedPathException(ex.getMessage()); } return false; }

@anmolnar Great work. you are so sweet! it's very elegant especially when we want to transfer the result value.
Cc:Thanks @eolivelli for mentioning this good option:CompletableFuture again.

phunt · 2019-03-13T04:23:53Z

zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java

@@ -55,14 +57,18 @@ public CliCommand parse(String[] cmdArgs) throws CliParseException {
    @Override
    public boolean exec() throws CliException {
        String path = args[1];
+        CountDownLatch latch = new CountDownLatch(1);
+        final int[] resultCode = new int[1];


Why is an array necessary here? Wouldn't a plain integer be sufficient?

when use a plain integer, a anonymous Inner Class needs it to be modified with final ,then we cannot assignment and transfer the result outside.

anmolnar

+1 LGTM

maoling · 2019-04-03T02:39:01Z

ping @anmolnar

anmolnar · 2019-04-03T08:45:47Z

@phunt @eolivelli Would you like to take another look?

eolivelli · 2019-04-03T11:42:34Z

zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java

@@ -30,6 +35,7 @@

    private static Options options = new Options();
    private String[] args;
+    public static final int CONNECTION_TIMEOUT = 30000;//30s


What about 'SYNC_TIMEOUT' ?
It is not used for 'comnection'

Please use TimeUnit so that it is self documenting.

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

TimeUnit.SECONDS.toMillis(30L)

eolivelli · 2019-04-03T11:43:57Z

zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java

+                int resultCode = cf.get(CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS);
+                out.println("Sync returned " + resultCode);
+            } catch (TimeoutException ex) {
+                out.println("Sync is timeout within " +  CONNECTION_TIMEOUT + " ms");


Should we throw an error?
The command may not have succeeded

@eolivelli pardon me.I may not get your idea clearly.
Do you mean that throwing this TimeoutException?

@maoling yes

Does this code need it's own 'try' block? Can this catch be moved to the outer 'try' statement?

anmolnar · 2019-04-04T08:38:39Z

zookeeper-server/src/main/java/org/apache/zookeeper/cli/SyncCommand.java

        } catch (IllegalArgumentException ex) {
            throw new MalformedPathException(ex.getMessage());
+        } catch (InterruptedException | ExecutionException ex) {


About catching InterruptedException: http://www.javapractices.com/topic/TopicAction.do?Id=251
We don't do anywhere else in our code which maybe not a good practice.
What do you think @belugabehr ?

I thought about it.
In this specific case we don't care. The CLI will exit.

I follow an example from this:
https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/cli/SetQuotaCommand.java#L82

Got it. Makes sense.

When you catch the Interrupted exception, it clears the Thread's state. I would separate the catch into two blocks and be sure to reinstate the flag to 'interrupted' so that any other code will also quickly exit.

} catch (InterruptedException ie) { Thread.currentThread().interrupt(); throw new CliWrapperException(ie); }

Yes but in this case it is not really needed.
We can add it but not so important

I think it's always best to do it the "right" way... it will insulate the code from changes later when it may become needed.

It's missing from everywhere else in the code unfortunately.
There's no harm adding it here anyway.

Yep, better to add.
Better to file a JIRA for fixing it every where

maoling · 2019-04-04T16:08:09Z

…of handling the InterruptedException

maoling · 2019-05-23T09:33:52Z

ping @anmolnar @eolivelli
Could you plz take another look?

eolivelli

Looks good

anmolnar · 2019-05-23T15:42:08Z

retest maven build

- Thanks the original work from [arshad.mohammad ](https://issues.apache.org/jira/secure/ViewProfile.jspa?name=arshad.mohammad) - more details in [ZOOKEEPER-2694](https://issues.apache.org/jira/browse/ZOOKEEPER-2694) Author: maoling <maoling199210191@sina.com> Reviewers: eolivelli@apache.org, andor@apache.org Closes #823 from maoling/ZOOKEEPER-2694 and squashes the following commits: 41723cd [maoling] print the resultCode when sync has failed 95123c0 [maoling] make the Sync more user-friendly a3ce170 [maoling] SYNC_TIMEOUT & use the TimeUnit.SECONDS.toMillis & a responsible way of handling the InterruptedException df0bf73 [maoling] CliWrapperException 52c3ad0 [maoling] change CountDownLatch to CompletableFuture 0cc1edd [maoling] add the wait_timeout: 30s cf01294 [maoling] ZOOKEEPER-2694:sync CLI command does not wait for result from server (cherry picked from commit e2bb6e8) Signed-off-by: Andor Molnar <andor@apache.org>

anmolnar · 2019-05-23T16:43:59Z

Patch committed to 3.5 and master branches. Thanks @maoling !

- Thanks the original work from [arshad.mohammad ](https://issues.apache.org/jira/secure/ViewProfile.jspa?name=arshad.mohammad) - more details in [ZOOKEEPER-2694](https://issues.apache.org/jira/browse/ZOOKEEPER-2694) Author: maoling <maoling199210191@sina.com> Reviewers: eolivelli@apache.org, andor@apache.org Closes apache#823 from maoling/ZOOKEEPER-2694 and squashes the following commits: 41723cd [maoling] print the resultCode when sync has failed 95123c0 [maoling] make the Sync more user-friendly a3ce170 [maoling] SYNC_TIMEOUT & use the TimeUnit.SECONDS.toMillis & a responsible way of handling the InterruptedException df0bf73 [maoling] CliWrapperException 52c3ad0 [maoling] change CountDownLatch to CompletableFuture 0cc1edd [maoling] add the wait_timeout: 30s cf01294 [maoling] ZOOKEEPER-2694:sync CLI command does not wait for result from server

maoling force-pushed the ZOOKEEPER-2694 branch from 2908251 to be831e2 Compare February 19, 2019 06:02

maoling closed this Mar 10, 2019

maoling reopened this Mar 10, 2019

anmolnar requested a review from afine March 12, 2019 18:18

anmolnar reviewed Mar 12, 2019

View reviewed changes

eolivelli requested changes Mar 12, 2019

View reviewed changes

phunt reviewed Mar 13, 2019

View reviewed changes

maoling force-pushed the ZOOKEEPER-2694 branch 3 times, most recently from c219e8b to fe6bc5c Compare March 20, 2019 11:22

maoling closed this Mar 20, 2019

maoling reopened this Mar 20, 2019

anmolnar approved these changes Mar 21, 2019

View reviewed changes

maoling force-pushed the ZOOKEEPER-2694 branch from fe6bc5c to c5c1912 Compare March 24, 2019 11:34

eolivelli requested changes Apr 3, 2019

View reviewed changes

anmolnar reviewed Apr 4, 2019

View reviewed changes

maoling force-pushed the ZOOKEEPER-2694 branch from c5c1912 to a781d4b Compare April 8, 2019 01:44

maoling closed this Apr 8, 2019

maoling reopened this Apr 8, 2019

maoling force-pushed the ZOOKEEPER-2694 branch from a781d4b to 7908bdf Compare April 15, 2019 11:28

maoling added 7 commits May 21, 2019 10:48

ZOOKEEPER-2694:sync CLI command does not wait for result from server

cf01294

add the wait_timeout: 30s

0cc1edd

change CountDownLatch to CompletableFuture

52c3ad0

CliWrapperException

df0bf73

SYNC_TIMEOUT & use the TimeUnit.SECONDS.toMillis & a responsible way …

a3ce170

…of handling the InterruptedException

make the Sync more user-friendly

95123c0

print the resultCode when sync has failed

41723cd

maoling force-pushed the ZOOKEEPER-2694 branch from 7908bdf to 41723cd Compare May 21, 2019 03:41

eolivelli approved these changes May 23, 2019

View reviewed changes

asfgit closed this in e2bb6e8 May 23, 2019

ZOOKEEPER-2694:sync CLI command does not wait for result from server #823

ZOOKEEPER-2694:sync CLI command does not wait for result from server #823

Conversation

maoling commented Feb 19, 2019

anmolnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolnar Mar 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolnar left a comment

Choose a reason for hiding this comment

maoling commented Apr 3, 2019

anmolnar commented Apr 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maoling commented Apr 4, 2019

maoling commented May 23, 2019

eolivelli left a comment

Choose a reason for hiding this comment

anmolnar commented May 23, 2019

anmolnar commented May 23, 2019

anmolnar Mar 19, 2019 •

edited