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
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 46 additions & 0 deletions server/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java
Expand Up @@ -19,6 +19,8 @@

package org.elasticsearch.cli;

import java.io.Closeable;
import java.io.IOException;
import java.util.Arrays;
import java.util.LinkedHashMap;
import java.util.Map;
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@bizybot bizybot Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Throwable th = null;
for (Closeable object : subcommands.values()) {
try {
if (object != null) {
object.close();
}
} catch (Throwable t) {
addSuppressed(th, t);
if (th == null) {
th = t;
}
}
}
if (th != null) {
throw reThrowAlways(th);
}
}

// Following methods are similar to IOUtils,
// avoiding lucene dependency in CLI.
private static void addSuppressed(Throwable t, Throwable suppressed) {
if (t != null && suppressed != null) {
t.addSuppressed(suppressed);
}
}

private static Error reThrowAlways(Throwable th) throws IOException, RuntimeException {
if (th instanceof IOException) {
throw (IOException) th;
}

if (th instanceof RuntimeException) {
throw (RuntimeException) th;
}

if (th instanceof Error) {
throw (Error) th;
}

throw new RuntimeException(th);
}
}
36 changes: 36 additions & 0 deletions server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java
Expand Up @@ -21,6 +21,14 @@

import joptsimple.OptionSet;
import org.junit.Before;
import org.mockito.Mockito;

import java.io.IOException;

import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;

public class MultiCommandTests extends CommandTestCase {

Expand Down Expand Up @@ -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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, Thank you.

Command spySubCommand2 = spy(new DummySubCommand());
multiCommand.subcommands.put("command1", spySubCommand1);
multiCommand.subcommands.put("command2", spySubCommand2);
multiCommand.close();
verify(spySubCommand1, times(1)).close();
verify(spySubCommand2, times(1)).close();
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment about not really needing mockito

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, Thank you.

Mockito.doThrow(new IOException()).when(subCommand1).close();
Command spySubCommand2 = spy(new DummySubCommand());
multiCommand.subcommands.put("command1", subCommand1);
multiCommand.subcommands.put("command2", spySubCommand2);
IOException ioe = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use expectThrows here instead of the try catch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thank you.

try {
multiCommand.close();
} catch (IOException e) {
ioe = e;
}
// verify exception is thrown, as well as other non failed sub-commands closed
// properly.
assertNotNull("Expected IOException", ioe);
verify(spySubCommand2, times(1)).close();
}
}