CLI: Close subcommands in MultiCommand by bizybot · Pull Request #28954 · elastic/elasticsearch

bizybot · 2018-03-09T03:20:33Z

Changes are done in MultiCommand, to override the close method and calling close on each subcommand to release resources.

… properly Changes done to override close method and call close on subcommands. Closes elastic#28953

rjernst

I left a couple comments. Also, please reword the PR title to indicate what is changing, eg "CLI: Close subcommands in MultiCommand". And for future reference, there is no need to create an associated issue with a PR.

rjernst · 2018-03-09T04:47:48Z

server/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java

+
+    @Override
+    public void close() throws IOException {
+        if (subcommands.isEmpty()) {


I don't think close() is the place to be erroring if subcommands were never setup. This would already happen in execute.

Addressed the review comment, by removing checks in the close method. Thank you.

rjernst · 2018-03-09T04:48:39Z

server/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java

+        if (subcommands.isEmpty()) {
+            throw new IllegalStateException("No subcommands configured");
+        }
+        for (Command command : subcommands.values()) {


Please use IOUtils.close() instead of iterating over the subcommands, as this will ensure all subcommands are closed, even on failures.

Hi Ryan, Not sure but is IOUtils from org.apache.lucene.util package? and can CLI be dependent on the Lucene utils? Please suggest so I can add the dependency. Thank you.

I forgot cli classes were moved out to their own jar. We definitely shouldn't add lucene core just to get IOUtils. But we do need to mimic the behavior here, otherwise one command failing to close could cause the others not to close.

Sure I will take it up and add similar behavior as in IOUtils#close. Thank you.

rjernst · 2018-03-09T05:17:40Z

One more thing: please add a test for this to MultiCommandTests.

elasticmachine · 2018-03-09T10:14:50Z

Pinging @elastic/es-core-infra

jasontedor · 2018-03-09T13:30:42Z

server/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java

+            throw new IllegalStateException("No subcommands configured");
+        }
+        for (Command command : subcommands.values()) {
+            if (command != null) {


If a command is null there is a problem; let us not be lenient like this.

Handling sub command close exceptions, so others can be closed and then rethrow exception. Adding unit tests. Closes elastic#28953

jasontedor · 2018-03-13T13:29:46Z

server/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java

    }
+
+    @Override
+    public void close() throws IOException, RuntimeException {


I don't think we should copy this here. I think if we are going to copy it, it should be available everywhere and replace the usage of the Lucene IOUtils. I opened #29012.

Thanks, Jason. I was not sure if we needed other utils but looks like from your description of the PR there are others who use the same across source code.

Just one question, around can CLI be dependent on elasticsearch-core? In current project dependencies, it is not listed. I can update my change to depend on the internal IOUtils.

CLI now depends on elasticsearch-core, for IOUtils. Closes elastic#28953

jaymode

Left a few comments about the testing. I also added the v6.3.0 label as this should be backported to 6.x.

jaymode · 2018-03-14T15:04:04Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

+        Command spySubCommand2 = spy(new DummySubCommand());
+        multiCommand.subcommands.put("command1", subCommand1);
+        multiCommand.subcommands.put("command2", spySubCommand2);
+        IOException ioe = null;


use expectThrows here instead of the try catch

Done. Thank you.

jaymode · 2018-03-14T15:07:38Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

    }
+
+    public void testClose() throws Exception {
+        Command spySubCommand1 = spy(new DummySubCommand());


I like mockito when it is necessary but sometimes it isn't and I feel that way here. I'd suggest having an AtomicBoolean or AtomicInteger that gets modified in the close method of a anonymous Command class. Then you can assert on that value.

Done, Thank you.

jaymode · 2018-03-14T15:07:56Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

+    }
+
+    public void testCloseWhenSubCommandCloseThrowsException() throws Exception {
+        Command subCommand1 = mock(DummySubCommand.class);


same comment about not really needing mockito

Done, Thank you.

Closes elastic#28953

bizybot · 2018-03-15T02:13:52Z

@elasticmachine Test this, please.

jaymode

I left some more comments

jaymode · 2018-03-15T02:45:55Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java


    static class DummyMultiCommand extends MultiCommand {
+
+        AtomicBoolean closed = new AtomicBoolean();


make this field final and add a line after for readability

jaymode · 2018-03-15T02:46:16Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

        DummyMultiCommand() {
            super("A dummy multi command", () -> {});
        }
+        @Override


This is a personal preference but given that we're adding to this class, I find it hard to read without empty lines between methods/constructor/member fields. Can you please add them?

jaymode · 2018-03-15T02:50:56Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

    }

    static class DummySubCommand extends Command {
+        AtomicBoolean throwsExceptionOnClose = new AtomicBoolean();


why does this need to be an AtomicBoolean? I think it can just be a final boolean

jaymode · 2018-03-15T02:51:07Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java


    static class DummySubCommand extends Command {
+        AtomicBoolean throwsExceptionOnClose = new AtomicBoolean();
+        AtomicBoolean closed = new AtomicBoolean();


make this final

jaymode · 2018-03-15T02:52:19Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

        }
+        DummySubCommand(boolean throwsExceptionOnClose) {
+            super("A dummy subcommand", () -> {});
+            this.throwsExceptionOnClose.compareAndSet(false, throwsExceptionOnClose);


when you use compareAndSet you need to check the value returned by this method. In this case there is something really wrong if it returns false.

jaymode · 2018-03-15T03:22:08Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

+    }
+
+    public void testCloseWhenSubCommandCloseThrowsException() {
+        boolean throwExceptionWhenClosed = true;


It would be nice to test the other scenarios or add some randomization to this test in terms of which subcommand(s) throws an exception:

final boolean command1Throws = randomBoolean(); final boolean command2Throws = randomBoolean(); ... assertTrue(command1Throws ^ subCommand1.closed.get()); assertTrue(command2Throws ^ subCommand2.closed.get());

jaymode · 2018-03-15T03:23:18Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

+        multiCommand.subcommands.put("command1", subCommand1);
+        multiCommand.subcommands.put("command2", subCommand2);
+        multiCommand.close();
+        assertTrue("MultiCommand must have been closed when close method is invoked", multiCommand.closed.get());


for me must have been closed points me in the wrong direction; I'd use should have been or was not closed

Closes elastic#28953

rjernst

I left a couple more comments. No need from me for another review if you agree with them all. Thanks for all the iterations!

rjernst · 2018-03-15T05:15:24Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

+        @Override
+        public void close() throws IOException {
+            super.close();
+            if (!this.closed.compareAndSet(false, true)) {


Typically we use == false for negations (much easier to spot when glancing through some code).

Yes, sure. Thank you. I will disable this in sonarLint which I use where it flags it as minor when == false.

rjernst · 2018-03-15T05:27:31Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

+        public void close() throws IOException {
+            super.close();
+            if (!this.closed.compareAndSet(false, true)) {
+                throw new IOException("DummyMultiCommand closed");


Shouldn't this be "DummyMultiCommand already closed"?

rjernst · 2018-03-15T05:27:47Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

+                throw new IOException();
+            } else {
+                if (!this.closed.compareAndSet(false, true)) {
+                    throw new IOException("DummySubCommand closed");


Same comments here as above.

rjernst · 2018-03-15T05:35:14Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

+        multiCommand.subcommands.put("command2", subCommand2);
+        // verify exception is thrown, as well as other non failed sub-commands closed
+        // properly.
+        IOException ioe = expectThrows(IOException.class, () -> multiCommand.close());


fyi, () -> multiCommand.close() can be multiCommand:close

rjernst · 2018-03-15T05:37:50Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

+
+        @Override
+        public void close() throws IOException {
+            if (throwsExceptionOnClose) {


If you have this condition+throw separate from setting closed (maybe rename closed to closeCalled) then you don't need the difficult to read xor logic in the last test.

Closes elastic#28953

bizybot · 2018-03-15T10:33:05Z

@elasticmachine Test this, please.

jaymode

I left a few minor comments, otherwise this LGTM. Once that's addressed and CI is green you should be good to merge this in!

jaymode · 2018-03-15T13:47:31Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

+        public void close() throws IOException {
+            super.close();
+            if (this.closed.compareAndSet(false, true) == false) {
+                throw new IOException("DummyMultiCommand already closed");


IMO this is not really an issue with IO so IOException doesn't make much sense; how about we change this to an IllegalStateException?

jaymode · 2018-03-15T13:47:59Z

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

+        @Override
+        public void close() throws IOException {
+            if (this.closeCalled.compareAndSet(false, true) == false) {
+                throw new IOException("DummySubCommand already closed");


same thing here, how about using IllegalStateException

Closes elastic#28953

* es/master: (97 commits) Clarify requirements of strict date formats. (#29090) Clarify that dates are always rendered as strings. (#29093) Compilation fix for #29067 [Docs] Fix link to Grok patterns (#29088) Store offsets in index prefix fields when stored in the parent field (#29067) Fix starting on Windows from another drive (#29086) Use removeTask instead of finishTask in PersistentTasksClusterService (#29055) Added minimal docs for reindex api in java-api docs Allow overriding JVM options in Windows service (#29044) Clarify how to set compiler and runtime JDKs (#29101) CLI: Close subcommands in MultiCommand (#28954) TEST: write ops should execute under shard permit (#28966) [DOCS] Add X-Pack upgrade details (#29038) Revert "Improve error message for installing plugin (#28298)" Docs: HighLevelRestClient#exists (#29073) Validate regular expressions in dynamic templates. (#29013) [Tests] Fix GetResultTests and DocumentFieldTests failures (#29083) Reenable LiveVersionMapTests.testRamBytesUsed on Java 9. (#29063) Mute failing GetResultTests and DocumentFieldTests Improve error message for installing plugin (#28298) ...

* CLI Command: MultiCommand must close subcommands to release resources properly - Changes are done to override the close method and call close on subcommands using IOUtils#close - Unit Test Closes #28953

* es/6.x: (46 commits) Docs: Add note about missing mapping for doc values field (#29036) [DOCS] Removed 6.1.4, 6.2.2, and 6.2.3 coming tags Remove BytesArray and BytesReference usage from XContentFactory (#29151) Fix BWC issue for PreSyncedFlushResponse Add pluggable XContentBuilder writers and human readable writers (#29120) Add unreleased version 6.2.4 (#29171) Add unreleased version 6.1.5 (#29168) Add a note about using the `retry_failed` flag before accepting data loss (#29160) Fix typo in percolate-query.asciidoc (#29155) Add release notes for 6.1.4 and 6.2.3 Require HTTP::Tiny 0.070 for release notes script REST high-level client: add clear cache API (#28866) Relax remote check for bwc project checkouts (#28666) Set Java 9 checkstyle to depend on checkstyle conf (#28383) Docs: Add example of resetting index setting (#29048) Plugins: Fix module name conflict check for meta plugins (#29146) Build: Fix meta plugin bundled plugin names (#29147) Build: Simplify rest spec hack configuration (#29149) CLI: Close subcommands in MultiCommand (#28954) Build: Fix meta modules to not install as plugin in tests (#29150) ...

Yogesh Gaikwad added 2 commits March 9, 2018 12:42

CLI Command: MultiCommand must close subcommands to release resources…

39770da

… properly Changes done to override close method and call close on subcommands. Closes elastic#28953

Merge branch 'master' into fix/28953

be11085

bizybot requested review from jaymode and rjernst March 9, 2018 03:20

bizybot added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v7.0.0 labels Mar 9, 2018

rjernst requested changes Mar 9, 2018

View reviewed changes

bizybot changed the title ~~CLI Command: MultiCommand must close subcommands to release resources properly~~ CLI: Close subcommands in MultiCommand Mar 9, 2018

jasontedor added :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts and removed :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache labels Mar 9, 2018

jasontedor reviewed Mar 9, 2018

View reviewed changes

Yogesh Gaikwad added 2 commits March 13, 2018 08:40

CLI: MultiCommand#close address review comments

bf6a3eb

Handling sub command close exceptions, so others can be closed and then rethrow exception. Adding unit tests. Closes elastic#28953

Merge branch 'master' into fix/28953

33d78c4

jasontedor reviewed Mar 13, 2018

View reviewed changes

Yogesh Gaikwad added 3 commits March 14, 2018 09:49

Merge branch 'master' into fix/28953

63c3656

CLI: MultiCommand#close address review comments

52a71d9

CLI now depends on elasticsearch-core, for IOUtils. Closes elastic#28953

Merge branch 'master' into fix/28953

ee04957

bizybot added the review label Mar 13, 2018

Merge branch 'master' into fix/28953

103d175

jaymode requested changes Mar 14, 2018

View reviewed changes

jaymode added the v6.3.0 label Mar 14, 2018

Yogesh Gaikwad added 3 commits March 15, 2018 07:33

CLI: MultiCommand#close address review comments

050a9f1

Closes elastic#28953

Merge branch 'master' into fix/28953

b2ece8a

Merge branch 'master' into fix/28953

99dd6ce

jaymode requested changes Mar 15, 2018

View reviewed changes

Yogesh Gaikwad added 2 commits March 15, 2018 14:54

CLI: MultiCommand#close address review comments

4b42afd

Closes elastic#28953

CLI: MultiCommand#close address review comments

6eb66e7

Closes elastic#28953

rjernst approved these changes Mar 15, 2018

View reviewed changes

CLI: MultiCommand#close address review comments

91d785e

Closes elastic#28953

jaymode approved these changes Mar 15, 2018

View reviewed changes

Yogesh Gaikwad added 2 commits March 16, 2018 07:39

CLI: MultiCommand#close address review comments

862090e

Closes elastic#28953

Merge branch 'master' into fix/28953

f098095

bizybot merged commit a685784 into elastic:master Mar 15, 2018

bizybot deleted the fix/28953 branch March 15, 2018 22:59

bizybot added the backport pending label Mar 15, 2018

bizybot added >bug and removed backport pending labels Mar 20, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020


		static class DummyMultiCommand extends MultiCommand {

		AtomicBoolean closed = new AtomicBoolean();

Conversation

bizybot commented Mar 9, 2018

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst commented Mar 9, 2018

Uh oh!

elasticmachine commented Mar 9, 2018

Uh oh!

jasontedor Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bizybot Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaymode left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

bizybot commented Mar 15, 2018

Uh oh!

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

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!

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

Choose a reason for hiding this comment

Uh oh!

jasontedor Mar 9, 2018 •

edited

Loading

bizybot Mar 13, 2018 •

edited

Loading

jaymode left a comment •

edited

Loading