Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI: Close subcommands in MultiCommand #28954

Merged
merged 16 commits into from Mar 15, 2018

Conversation

Projects
None yet
6 participants
@bizybot
Copy link
Contributor

commented Mar 9, 2018

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

bizybot added some commits Mar 9, 2018

CLI Command: MultiCommand must close subcommands to release resources…
… properly

Changes done to override close method and call close on subcommands.

Closes #28953
@rjernst
Copy link
Member

left a comment

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.


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

This comment has been minimized.

Copy link
@rjernst

rjernst Mar 9, 2018

Member

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

This comment has been minimized.

Copy link
@bizybot

bizybot Mar 13, 2018

Author Contributor

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

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

This comment has been minimized.

Copy link
@rjernst

rjernst Mar 9, 2018

Member

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

This comment has been minimized.

Copy link
@bizybot

bizybot Mar 9, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@rjernst

rjernst Mar 9, 2018

Member

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.

This comment has been minimized.

Copy link
@bizybot

bizybot Mar 9, 2018

Author Contributor

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

@rjernst

This comment has been minimized.

Copy link
Member

commented Mar 9, 2018

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

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

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Mar 9, 2018

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

This comment has been minimized.

Copy link
@jasontedor

jasontedor Mar 9, 2018

Member

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

bizybot added some commits Mar 12, 2018

CLI: MultiCommand#close address review comments
Handling sub command close exceptions, so others can be closed
and then rethrow exception.
Adding unit tests.

Closes #28953
@@ -74,4 +76,48 @@ protected void execute(Terminal terminal, OptionSet options) throws Exception {
}
subcommand.mainWithoutErrorHandling(Arrays.copyOfRange(args, 1, args.length), terminal);
}

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

This comment has been minimized.

Copy link
@jasontedor

jasontedor Mar 13, 2018

Member

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.

This comment has been minimized.

Copy link
@bizybot

bizybot Mar 13, 2018

Author Contributor

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.

bizybot added some commits Mar 13, 2018

CLI: MultiCommand#close address review comments
CLI now depends on elasticsearch-core, for IOUtils.

Closes #28953

@bizybot bizybot added the review label Mar 13, 2018

@jaymode
Copy link
Member

left a comment

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

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

This comment has been minimized.

Copy link
@jaymode

jaymode Mar 14, 2018

Member

use expectThrows here instead of the try catch

This comment has been minimized.

Copy link
@bizybot

bizybot Mar 14, 2018

Author Contributor

Done. Thank you.

@@ -102,4 +110,32 @@ public void testSubcommandArguments() throws Exception {
assertFalse(output, output.contains("command1"));
assertTrue(output, output.contains("Arguments: [foo, bar]"));
}

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

This comment has been minimized.

Copy link
@jaymode

jaymode Mar 14, 2018

Member

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.

This comment has been minimized.

Copy link
@bizybot

bizybot Mar 14, 2018

Author Contributor

Done, Thank you.

}

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

This comment has been minimized.

Copy link
@jaymode

jaymode Mar 14, 2018

Member

same comment about not really needing mockito

This comment has been minimized.

Copy link
@bizybot

bizybot Mar 14, 2018

Author Contributor

Done, Thank you.

@jaymode jaymode added the v6.3.0 label Mar 14, 2018

@bizybot

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2018

@elasticmachine Test this, please.

@jaymode
Copy link
Member

left a comment

I left some more comments

public class MultiCommandTests extends CommandTestCase {

static class DummyMultiCommand extends MultiCommand {

AtomicBoolean closed = new AtomicBoolean();

This comment has been minimized.

Copy link
@jaymode

jaymode Mar 15, 2018

Member

make this field final and add a line after for readability

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

This comment has been minimized.

Copy link
@jaymode

jaymode Mar 15, 2018

Member

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?

}

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

This comment has been minimized.

Copy link
@jaymode

jaymode Mar 15, 2018

Member

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

}

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

This comment has been minimized.

Copy link
@jaymode

jaymode Mar 15, 2018

Member

make this final

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

This comment has been minimized.

Copy link
@jaymode

jaymode Mar 15, 2018

Member

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.

}

public void testCloseWhenSubCommandCloseThrowsException() {
boolean throwExceptionWhenClosed = true;

This comment has been minimized.

Copy link
@jaymode

jaymode Mar 15, 2018

Member

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());
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());

This comment has been minimized.

Copy link
@jaymode

jaymode Mar 15, 2018

Member

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

bizybot added some commits Mar 15, 2018

@rjernst
Copy link
Member

left a comment

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

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

This comment has been minimized.

Copy link
@rjernst

rjernst Mar 15, 2018

Member

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

This comment has been minimized.

Copy link
@bizybot

bizybot Mar 15, 2018

Author Contributor

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

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

This comment has been minimized.

Copy link
@rjernst

rjernst Mar 15, 2018

Member

Shouldn't this be "DummyMultiCommand already closed"?

This comment has been minimized.

Copy link
@bizybot

bizybot Mar 15, 2018

Author Contributor

Done.

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

This comment has been minimized.

Copy link
@rjernst

rjernst Mar 15, 2018

Member

Same comments here as above.

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());

This comment has been minimized.

Copy link
@rjernst

rjernst Mar 15, 2018

Member

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


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

This comment has been minimized.

Copy link
@rjernst

rjernst Mar 15, 2018

Member

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.

@bizybot

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2018

@elasticmachine Test this, please.

@jaymode
Copy link
Member

left a comment

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!

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

This comment has been minimized.

Copy link
@jaymode

jaymode Mar 15, 2018

Member

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

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

This comment has been minimized.

Copy link
@jaymode

jaymode Mar 15, 2018

Member

same thing here, how about using IllegalStateException

bizybot added some commits Mar 15, 2018

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

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@bizybot bizybot deleted the bizybot:fix/28953 branch Mar 15, 2018

martijnvg added a commit that referenced this pull request Mar 16, 2018

Merge remote-tracking branch 'es/master' into ccr
* 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)
  ...

bizybot added a commit that referenced this pull request Mar 20, 2018

CLI: Close subcommands in MultiCommand (#28954)
* 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

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

martijnvg added a commit that referenced this pull request Mar 21, 2018

Merge remote-tracking branch 'es/6.x' into ccr-6.x
* 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)
  ...

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.