You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/12/04 19:11:08 UTC
[GitHub] [accumulo] Manno15 opened a new pull request #1824: WIP. Upgrade to jline3
Manno15 opened a new pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824
There is still some work to be done to this pull request but I wanted to get this out there to get some feedback in some areas.
I still need to do more robust testing on the shell and go back to a few areas that I commented out. I will also refactor/optimize areas if possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565533879
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
Review comment:
I don't believe this still occurs but I just noticed that CTRL-D will throw an `EndOfFileException` so I'll add a catch for that exception.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565315656
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -543,23 +551,17 @@ public int start() throws IOException {
if (!accumuloDir.exists() && !accumuloDir.mkdirs()) {
log.warn("Unable to make directory for history at {}", accumuloDir);
}
- try {
- final FileHistory history = new FileHistory(new File(historyPath));
- reader.setHistory(history);
- // Add shutdown hook to flush file history, per jline javadocs
- Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- try {
- history.flush();
- } catch (IOException e) {
- log.warn("Could not flush history to file.");
- }
- }));
- } catch (IOException e) {
- log.warn("Unable to load history file at {}", historyPath);
- }
+
+ // Remove Timestamps for history file. Fixes incompatibility issues
+ reader.unsetOpt(LineReader.Option.HISTORY_TIMESTAMPED);
+
+ // Set history file
+ reader.setVariable(LineReader.HISTORY_FILE, new File(historyPath));
// Turn Ctrl+C into Exception instead of JVM exit
- reader.setHandleUserInterrupt(true);
+ // Not 100% sure this is necessary anymore.
Review comment:
I did find that when I CTRL-C in the middle of the help command, it does work properly. As in it throws the interrupt exception but doesn't exit the process. It only exits if I CTRL-C when just sitting in the shell. So this signal handle is at least needed for that, unsure on why it still exits when CTRL-C not during a command.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-770102235
> I think this is probably mostly okay to merge now. However, I haven't done any manual testing... only code inspection and running tests. So, we should be careful to observe any behavioral changes to the shell as we continue development/testing on 2.1.
Understandable. Fortunately, most of the commands themselves have stayed the same. The things that have changed the most is tab completion and how the history file is set. Those I have tested thoroughly. The tests have also changed quite a bit and possible improvements could be made to those in future versions of Jline.
Another possible concern is how the terminal and reader will be set up on different machines. We let Jline determine the best terminal type and use the default implementations for other various features that maybe we would want to add later on. This isn't different from what we did in Jline2 so it isn't a big concern but I have already noticed several changes in functionality between `ConsoleReader` and the `terminal`/`lineReader` combo that is used now.
> There is one more thing that does need to be done before this can be merged, though:
the LICENSE file in the assembly tarball needs to be updated (assemble/src/main/resources/LICENSE) based on the upstream license information (there are only slight differences with https://github.com/jline/jline3/blob/jline-parent-3.17.1/LICENSE.txt
By the way, you may wish to consider updating to 3.19.0, in case some things have improved since you started working on this
I will add these Monday. I am curious to see what they changed in 3.19.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-768247124
I am running into an issue getting these changes to run with zookeeper 3.6.2 on fluo-uno. I will investigate. Specific error below:
<pre>
Exception in thread "shell" java.lang.UnsatisfiedLinkError: 'int org.fusesource.jansi.internal.CLibrary.ioctl(int, long, org.fusesource.jansi.internal.CLibrary$WinSize)'
at org.fusesource.jansi.internal.CLibrary.ioctl(Native Method)
at org.jline.terminal.impl.jansi.JansiNativePty.getSize(JansiNativePty.java:146)
at org.jline.terminal.impl.AbstractPosixTerminal.getSize(AbstractPosixTerminal.java:60)
at org.jline.terminal.Terminal.getBufferSize(Terminal.java:216)
at org.jline.reader.impl.LineReaderImpl.doDisplay(LineReaderImpl.java:746)
at org.jline.reader.impl.LineReaderImpl.<init>(LineReaderImpl.java:303)
at org.jline.reader.LineReaderBuilder.build(LineReaderBuilder.java:115)
at org.apache.accumulo.shell.Shell.config(Shell.java:275)
at org.apache.accumulo.shell.Shell.execute(Shell.java:520)
at org.apache.accumulo.start.Main.lambda$execKeyword$0(Main.java:126)
at java.base/java.lang.Thread.run(Thread.java:834)
</pre>
EDIT: My guess is for some reason LineReader is trying to default to using Jansi for the terminal and we don't have the correct libraries for it? I will push a commit that tells the terminal to not allow Jansi so it should default to a different type. I believe the old default type was xterm so not sure why the new zookeeper version updates the default to Jansi.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-770876560
I updated the license and upgraded to 3.19.0. There wasn't anything noticeable to the upgrade that would benefit us so no further changes from it. I will merge this today unless there are any objections.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r566456616
##########
File path: shell/src/test/java/org/apache/accumulo/shell/commands/HistoryCommandTest.java
##########
@@ -56,38 +62,41 @@ public void setUp() throws Exception {
expect(cl.hasOption("np")).andReturn(true);
replay(cl);
- History history = new MemoryHistory();
+ History history = new DefaultHistory();
history.add("foo");
history.add("bar");
baos = new ByteArrayOutputStream();
String input = String.format("!1%n"); // Construct a platform dependent new-line
- reader = new ConsoleReader(new ByteArrayInputStream(input.getBytes()), baos);
- reader.setHistory(history);
+ terminal = TerminalBuilder.builder().system(false)
+ .streams(new ByteArrayInputStream(input.getBytes()), baos).build();
+ reader = LineReaderBuilder.builder().history(history).terminal(terminal).build();
Review comment:
Makes sense! Thanks for the explanation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #1824: WIP. Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-754822950
I believe this pull request is ready for review. There are a couple of areas where I unsure of the proper route to go on. One is the use of `DumbTerminal` in [ShellIT](https://github.com/apache/accumulo/blob/61a1755547fc0f9fd6290eb4abe704b775657618/test/src/main/java/org/apache/accumulo/test/ShellIT.java#L166) and [ShellServerIT](https://github.com/Manno15/accumulo/blob/61a1755547fc0f9fd6290eb4abe704b775657618/test/src/main/java/org/apache/accumulo/test/ShellServerIT.java#L198). The other is [here](https://github.com/apache/accumulo/blob/61a1755547fc0f9fd6290eb4abe704b775657618/shell/src/main/java/org/apache/accumulo/shell/Shell.java#L563)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r566797031
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
Review comment:
From my testing, there is no reasonable way for a user to make input null beyond hitting CTRL-D, which sends a EOF exception. I interpret that as the user wanting to exit (which is standard for BASH but we can change that if we want). My latest commit reflects that idea.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r566765834
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
Review comment:
Another question I have, is if we want to exit on CTRL-D or for CTRL-C for that matter (exits only when just sitting in the terminal). I am in favor of not exiting on either of those two things but the current implementation is to exit the shell.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565446445
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -182,14 +186,19 @@ public String getErrorMessage() {
public TestOutputStream output;
public StringInputStream input;
public Shell shell;
+ public LineReader reader;
+ public Terminal terminal;
TestShell(String user, String rootPass, String instanceName, String zookeepers, File configFile)
throws IOException {
ClientInfo info = ClientInfo.from(configFile.toPath());
// start the shell
output = new TestOutputStream();
input = new StringInputStream();
- shell = new Shell(new ConsoleReader(input, output));
+ terminal = new DumbTerminal(input, output);
+ terminal.setSize(new Size(80, 24));
+ reader = LineReaderBuilder.builder().terminal(terminal).build();
+ shell = new Shell(reader);
Review comment:
I will look into it further once I get through the rest of the comments.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-768247124
I am running into an issue getting these changes to run with zookeeper 3.6.2 on fluo-uno. I will investigate. Specific error below:
<pre>
Exception in thread "shell" java.lang.UnsatisfiedLinkError: 'int org.fusesource.jansi.internal.CLibrary.ioctl(int, long, org.fusesource.jansi.internal.CLibrary$WinSize)'
at org.fusesource.jansi.internal.CLibrary.ioctl(Native Method)
at org.jline.terminal.impl.jansi.JansiNativePty.getSize(JansiNativePty.java:146)
at org.jline.terminal.impl.AbstractPosixTerminal.getSize(AbstractPosixTerminal.java:60)
at org.jline.terminal.Terminal.getBufferSize(Terminal.java:216)
at org.jline.reader.impl.LineReaderImpl.doDisplay(LineReaderImpl.java:746)
at org.jline.reader.impl.LineReaderImpl.<init>(LineReaderImpl.java:303)
at org.jline.reader.LineReaderBuilder.build(LineReaderBuilder.java:115)
at org.apache.accumulo.shell.Shell.config(Shell.java:275)
at org.apache.accumulo.shell.Shell.execute(Shell.java:520)
at org.apache.accumulo.start.Main.lambda$execKeyword$0(Main.java:126)
at java.base/java.lang.Thread.run(Thread.java:834)
</pre>
EDIT: My guess is for some reason LineReader is trying to default to using Jansi and we don't have the correct libraries for it? I will push a commit that tells the terminal to not allow Jansi so it should default to a different type. I believe the old default type was xterm so not sure why the new zookeeper version updates the default to Jansi.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r552905609
##########
File path: shell/src/main/java/org/apache/accumulo/shell/format/DeleterFormatter.java
##########
@@ -79,33 +79,36 @@ public String next() {
Mutation m = new Mutation(key.getRow());
String entryStr = formatEntry(next, isDoTimestamps());
boolean delete = force;
- try {
- if (!force) {
- shellState.getReader().flush();
+ if (!force) {
+ try {
+ shellState.getWriter().flush();
+ // this will cause end of file Exception for one of the formatter tests
String line = shellState.getReader().readLine("Delete { " + entryStr + " } ? ");
more = line != null;
delete = line != null && (line.equalsIgnoreCase("y") || line.equalsIgnoreCase("yes"));
Review comment:
Could the portion before `&&` be replaced with `more`?
```suggestion
delete = more && (line.equalsIgnoreCase("y") || line.equalsIgnoreCase("yes"));
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-769785635
@ctubbsii I believe I hit on everyones questions and suggestions. Do you have anything further to add or any concerns?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 merged pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 merged pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565239738
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/HiddenCommand.java
##########
@@ -42,9 +43,9 @@ public String description() {
public int execute(final String fullCommand, final CommandLine cl, final Shell shellState)
throws Exception {
if (rand.nextInt(10) == 0) {
- shellState.getReader().beep();
- shellState.getReader().println();
- shellState.getReader()
+ shellState.getTerminal().puts(InfoCmp.Capability.bell);
Review comment:
I have not but I will try on my end. This was how the devs on the mailing list said to do it. I never heard the bell on the old shell so I am not sure if I will hear this one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565218651
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
return exitCode;
} // User Canceled (Ctrl+D)
execCommand(input, disableAuthTimeout, false);
} catch (UserInterruptException uie) {
// User Cancelled (Ctrl+C)
- reader.println();
+ writer.println();
String partialLine = uie.getPartialLine();
if (partialLine == null || "".equals(uie.getPartialLine().trim())) {
// No content, actually exit
return exitCode;
}
} finally {
- reader.flush();
+ writer.flush();
}
}
}
public void shutdown() {
if (reader != null) {
- reader.close();
+ try {
+ terminal.close();
+ reader = null;
+ } catch (IOException e) {
+ e.printStackTrace();
Review comment:
I will add.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r566453946
##########
File path: shell/src/main/java/org/apache/accumulo/shell/format/DeleterFormatter.java
##########
@@ -79,33 +79,36 @@ public String next() {
Mutation m = new Mutation(key.getRow());
String entryStr = formatEntry(next, isDoTimestamps());
boolean delete = force;
- try {
- if (!force) {
- shellState.getReader().flush();
- String line = shellState.getReader().readLine("Delete { " + entryStr + " } ? ");
- more = line != null;
- delete = line != null && (line.equalsIgnoreCase("y") || line.equalsIgnoreCase("yes"));
+ String line;
+ if (!force) {
+ try {
+ shellState.getWriter().flush();
+ String prompt = "Delete { " + entryStr + " } ? ";
+ line = shellState.getReader().readLine(prompt);
+ } catch (EndOfFileException ignored) {
+ // Reached the end of file. Line is null to keep old functionality.
+ line = null;
}
- if (delete) {
- m.putDelete(key.getColumnFamily(), key.getColumnQualifier(),
- new ColumnVisibility(key.getColumnVisibility()), key.getTimestamp());
- try {
- writer.addMutation(m);
- } catch (MutationsRejectedException e) {
- log.error(e.toString());
- if (Shell.log.isTraceEnabled()) {
- for (ConstraintViolationSummary cvs : e.getConstraintViolationSummaries()) {
- log.trace(cvs.toString());
- }
+ more = line != null;
+ delete = line != null && (line.equalsIgnoreCase("y") || line.equalsIgnoreCase("yes"));
Review comment:
I think this suggestion was made previously, but may have been overlooked.
```suggestion
delete = more && (line.equalsIgnoreCase("y") || line.equalsIgnoreCase("yes"));
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565497825
##########
File path: shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java
##########
@@ -29,14 +29,14 @@
import java.io.PrintStream;
import java.nio.file.Files;
+import org.jline.reader.impl.LineReaderImpl;
Review comment:
I forgot to update this test to use a dumb terminal like the other tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-770071413
> > @ctubbsii I believe I hit on everyones questions and suggestions. Do you have anything further to add or any concerns?
>
> I can take another look today and re-run the ITs.
I re-ran all the ITs, and they all pass, except for a few known flaky ones.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565539829
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -543,23 +551,17 @@ public int start() throws IOException {
if (!accumuloDir.exists() && !accumuloDir.mkdirs()) {
log.warn("Unable to make directory for history at {}", accumuloDir);
}
- try {
- final FileHistory history = new FileHistory(new File(historyPath));
- reader.setHistory(history);
- // Add shutdown hook to flush file history, per jline javadocs
- Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- try {
- history.flush();
- } catch (IOException e) {
- log.warn("Could not flush history to file.");
- }
- }));
- } catch (IOException e) {
- log.warn("Unable to load history file at {}", historyPath);
- }
+
+ // Remove Timestamps for history file. Fixes incompatibility issues
+ reader.unsetOpt(LineReader.Option.HISTORY_TIMESTAMPED);
+
+ // Set history file
+ reader.setVariable(LineReader.HISTORY_FILE, new File(historyPath));
// Turn Ctrl+C into Exception instead of JVM exit
- reader.setHandleUserInterrupt(true);
+ // Not 100% sure this is necessary anymore.
Review comment:
This is because of the block below. Not sure if this is a feature that we want or not.
```java
catch (UserInterruptException uie) {
// User Cancelled (Ctrl+C)
reader.println();
writer.println();
String partialLine = uie.getPartialLine();
if (partialLine == null || "".equals(uie.getPartialLine().trim())) {
// No content, actually exit
return exitCode;
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-768247124
I am running into an issue getting these changes to run with zookeeper 3.6.2 on fluo-uno. I will investigate. Specific error below:
<pre>
Exception in thread "shell" java.lang.UnsatisfiedLinkError: 'int org.fusesource.jansi.internal.CLibrary.ioctl(int, long, org.fusesource.jansi.internal.CLibrary$WinSize)'
at org.fusesource.jansi.internal.CLibrary.ioctl(Native Method)
at org.jline.terminal.impl.jansi.JansiNativePty.getSize(JansiNativePty.java:146)
at org.jline.terminal.impl.AbstractPosixTerminal.getSize(AbstractPosixTerminal.java:60)
at org.jline.terminal.Terminal.getBufferSize(Terminal.java:216)
at org.jline.reader.impl.LineReaderImpl.doDisplay(LineReaderImpl.java:746)
at org.jline.reader.impl.LineReaderImpl.<init>(LineReaderImpl.java:303)
at org.jline.reader.LineReaderBuilder.build(LineReaderBuilder.java:115)
at org.apache.accumulo.shell.Shell.config(Shell.java:275)
at org.apache.accumulo.shell.Shell.execute(Shell.java:520)
at org.apache.accumulo.start.Main.lambda$execKeyword$0(Main.java:126)
at java.base/java.lang.Thread.run(Thread.java:834)
</pre>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565533879
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
Review comment:
I don't believe this still occurs but I will add it back because I don't have a great way to test and a println doesn't hurt. I also just noticed that CTRL-D will throw an `EndOfFileException` if I do it on a blank line so I'll add a catch for that exception. This might make that if statement redundant if that is the only situation input could be null.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565467945
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/HiddenCommand.java
##########
@@ -42,9 +43,9 @@ public String description() {
public int execute(final String fullCommand, final CommandLine cl, final Shell shellState)
throws Exception {
if (rand.nextInt(10) == 0) {
- shellState.getReader().beep();
- shellState.getReader().println();
- shellState.getReader()
+ shellState.getTerminal().puts(InfoCmp.Capability.bell);
Review comment:
Confirmed that the bell is there with these changes. Had to do a bit of settings adjustments in my terminal to make it audible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r552838112
##########
File path: server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java
##########
@@ -271,19 +273,23 @@ static boolean checkInit(VolumeManager fs, SiteConfiguration sconf, Configuratio
return false;
}
if (sconf.get(Property.INSTANCE_SECRET).equals(Property.INSTANCE_SECRET.getDefaultValue())) {
- ConsoleReader c = getConsoleReader();
- c.beep();
- c.println();
- c.println();
- c.println("Warning!!! Your instance secret is still set to the default,"
- + " this is not secure. We highly recommend you change it.");
- c.println();
- c.println();
- c.println("You can change the instance secret in accumulo by using:");
- c.println(" bin/accumulo " + org.apache.accumulo.server.util.ChangeSecret.class.getName());
- c.println("You will also need to edit your secret in your configuration"
- + " file by adding the property instance.secret to your"
- + " accumulo.properties. Without this accumulo will not operate" + " correctly");
+ LineReader c = getLineReader();
+ c.getTerminal().puts(InfoCmp.Capability.bell);
+ c.getTerminal().writer().println();
Review comment:
Sometimes this is done for readability, sometimes to reuse a buffer for performance to avoid unnecessary string concatenation (as in StringBuilder append methods), and sometimes to defer to environment-specific implementations (like cross-platform newline differences). It could also just be more convenient for a developer to copy/paste as many `println` calls as they want, rather than edit in a specific number of `\n` in several different String literals.
I'm not sure why it's used this way in this particular case, but probably some combination of readability and developer convenience. But, it probably also makes sense in this case to defer to the terminal's environment-specific implementation, since the terminal itself is intended as an abstraction around the command-line environment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565438050
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -182,14 +186,19 @@ public String getErrorMessage() {
public TestOutputStream output;
public StringInputStream input;
public Shell shell;
+ public LineReader reader;
+ public Terminal terminal;
TestShell(String user, String rootPass, String instanceName, String zookeepers, File configFile)
throws IOException {
ClientInfo info = ClientInfo.from(configFile.toPath());
// start the shell
output = new TestOutputStream();
input = new StringInputStream();
- shell = new Shell(new ConsoleReader(input, output));
+ terminal = new DumbTerminal(input, output);
+ terminal.setSize(new Size(80, 24));
+ reader = LineReaderBuilder.builder().terminal(terminal).build();
+ shell = new Shell(reader);
Review comment:
The gain is just more intuitive API semantics (possibly) and maybe exposing a more stable jline API... a terminal being a high level object and line reader a lower level component.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r564827915
##########
File path: server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java
##########
@@ -271,19 +273,23 @@ static boolean checkInit(VolumeManager fs, SiteConfiguration sconf, Configuratio
return false;
}
if (sconf.get(Property.INSTANCE_SECRET).equals(Property.INSTANCE_SECRET.getDefaultValue())) {
- ConsoleReader c = getConsoleReader();
- c.beep();
- c.println();
- c.println();
- c.println("Warning!!! Your instance secret is still set to the default,"
- + " this is not secure. We highly recommend you change it.");
- c.println();
- c.println();
- c.println("You can change the instance secret in accumulo by using:");
- c.println(" bin/accumulo " + org.apache.accumulo.server.util.ChangeSecret.class.getName());
- c.println("You will also need to edit your secret in your configuration"
- + " file by adding the property instance.secret to your"
- + " accumulo.properties. Without this accumulo will not operate" + " correctly");
+ LineReader c = getLineReader();
+ c.getTerminal().puts(InfoCmp.Capability.bell);
+ c.getTerminal().writer().println();
+ c.getTerminal().writer().println();
+
+ c.getTerminal().writer()
Review comment:
It might make sense to assign the writer to a new variable, so that way all this line wrapping doesn't occur.
```java
var w = c.getTerminal().writer();
w.println(...);
```
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -895,7 +904,7 @@ public void clonetableOffline() throws Exception {
ts.exec("scan", false, "TableOfflineException", true);
ts.exec("constraint --list -t " + clone, true, "VisibilityConstraint=2", true);
ts.exec("config -t " + clone + " -np", true, "123M", true);
- ts.exec("getsplits -t " + clone, true, "a\nb\nc\n");
+ ts.exec("getsplits -t " + clone, true, "abc");
Review comment:
These splits should be on separate lines. It seems like either this behavior has changed, or there's a problem with these tests.
##########
File path: shell/src/main/java/org/apache/accumulo/shell/format/DeleterFormatter.java
##########
@@ -79,33 +79,36 @@ public String next() {
Mutation m = new Mutation(key.getRow());
String entryStr = formatEntry(next, isDoTimestamps());
boolean delete = force;
- try {
- if (!force) {
- shellState.getReader().flush();
+ if (!force) {
+ try {
+ shellState.getWriter().flush();
+ // this will cause end of file Exception for one of the formatter tests
Review comment:
Which one? Why? Can it be fixed?
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
Review comment:
This println previously existed, so the user's bash shell or whatever, wouldn't print its prompt at the end of Accumulo's prompt, when existing by closing the input stream. I assume by removing this println, that no longer occurs? If it does, that should probably be put back.
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellIT.java
##########
@@ -207,9 +215,9 @@ public void addGetSplitsTest() throws IOException {
exec("addsplits arg", false, "java.lang.IllegalStateException: Not in a table context");
exec("createtable test", true);
exec("addsplits 1 \\x80", true);
- exec("getsplits", true, "1\n\\x80");
+ exec("getsplits", true, "1\\x80");
exec("getsplits -m 1", true, "1");
- exec("getsplits -b64", true, "MQ==\ngA==");
+ exec("getsplits -b64", true, "MQ==gA==");
Review comment:
If these tests pass without the newline, then that seems like it would indicate a breaking change to getsplits.
##########
File path: shell/src/test/java/org/apache/accumulo/shell/ShellConfigTest.java
##########
@@ -29,14 +29,14 @@
import java.io.PrintStream;
import java.nio.file.Files;
+import org.jline.reader.impl.LineReaderImpl;
Review comment:
Is it safe to use the impl here? Doing so seems fragile.
##########
File path: shell/src/main/java/org/apache/accumulo/shell/ShellCompletor.java
##########
@@ -45,13 +48,16 @@ public ShellCompletor(Token rootToken, Map<CompletionSet,Set<String>> options) {
@Override
@SuppressWarnings({"unchecked", "rawtypes"})
Review comment:
These warning suppressions might not make sense anymore, now that you've added the appropriate generics to the candidates list.
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -182,14 +186,19 @@ public String getErrorMessage() {
public TestOutputStream output;
public StringInputStream input;
public Shell shell;
+ public LineReader reader;
+ public Terminal terminal;
TestShell(String user, String rootPass, String instanceName, String zookeepers, File configFile)
throws IOException {
ClientInfo info = ClientInfo.from(configFile.toPath());
// start the shell
output = new TestOutputStream();
input = new StringInputStream();
- shell = new Shell(new ConsoleReader(input, output));
+ terminal = new DumbTerminal(input, output);
+ terminal.setSize(new Size(80, 24));
+ reader = LineReaderBuilder.builder().terminal(terminal).build();
+ shell = new Shell(reader);
Review comment:
I wonder if it would make more sense to pass in "terminal" instead of just the reader?
##########
File path: shell/src/test/java/org/apache/accumulo/shell/format/DeleterFormatterTest.java
##########
@@ -133,6 +141,7 @@ public void testNo() throws IOException {
assertTrue(formatter.hasNext());
}
+ // Need to properly fix this test.
Review comment:
What's this comment about?
##########
File path: shell/src/test/java/org/apache/accumulo/shell/commands/HistoryCommandTest.java
##########
@@ -56,38 +62,41 @@ public void setUp() throws Exception {
expect(cl.hasOption("np")).andReturn(true);
replay(cl);
- History history = new MemoryHistory();
+ History history = new DefaultHistory();
history.add("foo");
history.add("bar");
baos = new ByteArrayOutputStream();
String input = String.format("!1%n"); // Construct a platform dependent new-line
- reader = new ConsoleReader(new ByteArrayInputStream(input.getBytes()), baos);
- reader.setHistory(history);
+ terminal = TerminalBuilder.builder().system(false)
+ .streams(new ByteArrayInputStream(input.getBytes()), baos).build();
+ reader = LineReaderBuilder.builder().history(history).terminal(terminal).build();
Review comment:
Is there a way to add the history directly to the terminal, and using its line reader, or do we have to create a new line reader separately from the terminal?
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/HistoryCommand.java
##########
@@ -39,11 +39,11 @@
public int execute(final String fullCommand, final CommandLine cl, final Shell shellState)
throws IOException {
if (cl.hasOption(clearHist.getOpt())) {
- shellState.getReader().getHistory().clear();
+ shellState.getReader().getHistory().purge();
} else {
- Iterator<Entry> source = shellState.getReader().getHistory().entries();
+ Iterator<History.Entry> source = shellState.getReader().getHistory().iterator();
Iterator<String> historyIterator = Iterators.transform(source,
- input -> String.format("%d: %s", input.index() + 1, input.value()));
+ input -> String.format("%d: %s", input.index() + 1, input.line()));
Review comment:
It looks like Iterator was used here previously because `.entries()` returned a type of `Iterator`. However, it now looks like `.getHistory()` returns a type of `Iterable`, which gives an opportunity to write this differently, using Java streams or something, and to avoid the use of Guava's `Iterators.transform()` in favor of Java built-ins. It's not important, though. It's fine either way.
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
return exitCode;
} // User Canceled (Ctrl+D)
execCommand(input, disableAuthTimeout, false);
} catch (UserInterruptException uie) {
// User Cancelled (Ctrl+C)
- reader.println();
+ writer.println();
String partialLine = uie.getPartialLine();
if (partialLine == null || "".equals(uie.getPartialLine().trim())) {
// No content, actually exit
return exitCode;
}
} finally {
- reader.flush();
+ writer.flush();
}
}
}
public void shutdown() {
if (reader != null) {
- reader.close();
+ try {
+ terminal.close();
+ reader = null;
+ } catch (IOException e) {
+ e.printStackTrace();
Review comment:
I concur. We don't generally just print stack traces with `e.printStackTrace()`.
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/HiddenCommand.java
##########
@@ -42,9 +43,9 @@ public String description() {
public int execute(final String fullCommand, final CommandLine cl, final Shell shellState)
throws Exception {
if (rand.nextInt(10) == 0) {
- shellState.getReader().beep();
- shellState.getReader().println();
- shellState.getReader()
+ shellState.getTerminal().puts(InfoCmp.Capability.bell);
Review comment:
Have you tested the bell to make sure it actually rings? (might depend on your local development environment).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r564823374
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -543,23 +551,17 @@ public int start() throws IOException {
if (!accumuloDir.exists() && !accumuloDir.mkdirs()) {
log.warn("Unable to make directory for history at {}", accumuloDir);
}
- try {
- final FileHistory history = new FileHistory(new File(historyPath));
- reader.setHistory(history);
- // Add shutdown hook to flush file history, per jline javadocs
- Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- try {
- history.flush();
- } catch (IOException e) {
- log.warn("Could not flush history to file.");
- }
- }));
- } catch (IOException e) {
- log.warn("Unable to load history file at {}", historyPath);
- }
+
+ // Remove Timestamps for history file. Fixes incompatibility issues
+ reader.unsetOpt(LineReader.Option.HISTORY_TIMESTAMPED);
+
+ // Set history file
+ reader.setVariable(LineReader.HISTORY_FILE, new File(historyPath));
// Turn Ctrl+C into Exception instead of JVM exit
- reader.setHandleUserInterrupt(true);
+ // Not 100% sure this is necessary anymore.
Review comment:
I believe CTRL-C still worked without it but I will double check tomorrow to verify.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r566744259
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
Review comment:
I will do more testing to confirm then push out my findings, CTRL-D for sure is hit by the exception and everything looks good there. I just need to make sure there is no other case input will equal null and to see what happens if something like that does happen.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565408422
##########
File path: shell/src/main/java/org/apache/accumulo/shell/format/DeleterFormatter.java
##########
@@ -79,33 +79,36 @@ public String next() {
Mutation m = new Mutation(key.getRow());
String entryStr = formatEntry(next, isDoTimestamps());
boolean delete = force;
- try {
- if (!force) {
- shellState.getReader().flush();
+ if (!force) {
+ try {
+ shellState.getWriter().flush();
+ // this will cause end of file Exception for one of the formatter tests
Review comment:
It is possible that Jline3 changed something behind the scenes. On the main branch, line returns null when trying to read the prompt. Currently, it reads the end of file. Not sure what the best approach to fix it yet but I am trying to get it to where it returns null.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r566034023
##########
File path: shell/src/main/java/org/apache/accumulo/shell/format/DeleterFormatter.java
##########
@@ -79,33 +79,36 @@ public String next() {
Mutation m = new Mutation(key.getRow());
String entryStr = formatEntry(next, isDoTimestamps());
boolean delete = force;
- try {
- if (!force) {
- shellState.getReader().flush();
+ if (!force) {
+ try {
+ shellState.getWriter().flush();
+ // this will cause end of file Exception for one of the formatter tests
Review comment:
I added a potential fix for this in my latest commit. It is probably good enough without completely rewriting `DeleterFormatter`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r566743300
##########
File path: shell/src/main/java/org/apache/accumulo/shell/format/DeleterFormatter.java
##########
@@ -79,33 +79,36 @@ public String next() {
Mutation m = new Mutation(key.getRow());
String entryStr = formatEntry(next, isDoTimestamps());
boolean delete = force;
- try {
- if (!force) {
- shellState.getReader().flush();
- String line = shellState.getReader().readLine("Delete { " + entryStr + " } ? ");
- more = line != null;
- delete = line != null && (line.equalsIgnoreCase("y") || line.equalsIgnoreCase("yes"));
+ String line;
+ if (!force) {
+ try {
+ shellState.getWriter().flush();
+ String prompt = "Delete { " + entryStr + " } ? ";
+ line = shellState.getReader().readLine(prompt);
+ } catch (EndOfFileException ignored) {
+ // Reached the end of file. Line is null to keep old functionality.
+ line = null;
}
- if (delete) {
- m.putDelete(key.getColumnFamily(), key.getColumnQualifier(),
- new ColumnVisibility(key.getColumnVisibility()), key.getTimestamp());
- try {
- writer.addMutation(m);
- } catch (MutationsRejectedException e) {
- log.error(e.toString());
- if (Shell.log.isTraceEnabled()) {
- for (ConstraintViolationSummary cvs : e.getConstraintViolationSummaries()) {
- log.trace(cvs.toString());
- }
+ more = line != null;
+ delete = line != null && (line.equalsIgnoreCase("y") || line.equalsIgnoreCase("yes"));
Review comment:
You are correct. I will add that in.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] lbschanno commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
lbschanno commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r564748561
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
return exitCode;
} // User Canceled (Ctrl+D)
execCommand(input, disableAuthTimeout, false);
} catch (UserInterruptException uie) {
// User Cancelled (Ctrl+C)
- reader.println();
+ writer.println();
String partialLine = uie.getPartialLine();
if (partialLine == null || "".equals(uie.getPartialLine().trim())) {
// No content, actually exit
return exitCode;
}
} finally {
- reader.flush();
+ writer.flush();
}
}
}
public void shutdown() {
if (reader != null) {
- reader.close();
+ try {
+ terminal.close();
+ reader = null;
+ } catch (IOException e) {
+ e.printStackTrace();
Review comment:
I suggest adding a log.warn or log.error here to capture the exception within the logs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r552755071
##########
File path: server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java
##########
@@ -271,19 +273,23 @@ static boolean checkInit(VolumeManager fs, SiteConfiguration sconf, Configuratio
return false;
}
if (sconf.get(Property.INSTANCE_SECRET).equals(Property.INSTANCE_SECRET.getDefaultValue())) {
- ConsoleReader c = getConsoleReader();
- c.beep();
- c.println();
- c.println();
- c.println("Warning!!! Your instance secret is still set to the default,"
- + " this is not secure. We highly recommend you change it.");
- c.println();
- c.println();
- c.println("You can change the instance secret in accumulo by using:");
- c.println(" bin/accumulo " + org.apache.accumulo.server.util.ChangeSecret.class.getName());
- c.println("You will also need to edit your secret in your configuration"
- + " file by adding the property instance.secret to your"
- + " accumulo.properties. Without this accumulo will not operate" + " correctly");
+ LineReader c = getLineReader();
+ c.getTerminal().puts(InfoCmp.Capability.bell);
+ c.getTerminal().writer().println();
Review comment:
Does anyone know why the print statements are like this as opposed to one/fewer prints with linebreaks (`\n`). It seems as though there might be a reason `println`s are used in this way, but it seems like it would be cleaner to consolidate this text into less prints if possible. I see that it was this way before this PR so its not quite related but I was just curious.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565484980
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellIT.java
##########
@@ -207,9 +215,9 @@ public void addGetSplitsTest() throws IOException {
exec("addsplits arg", false, "java.lang.IllegalStateException: Not in a table context");
exec("createtable test", true);
exec("addsplits 1 \\x80", true);
- exec("getsplits", true, "1\n\\x80");
+ exec("getsplits", true, "1\\x80");
exec("getsplits -m 1", true, "1");
- exec("getsplits -b64", true, "MQ==\ngA==");
+ exec("getsplits -b64", true, "MQ==gA==");
Review comment:
Same issue as above. Printed instead of PrintLn
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1824: WIP. Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-739136949
@Manno15 This is a great start. It looks like a beast of a problem, but it looks like you're headed in the right direction! The fact that so much of the API of JLine is subject to change makes me wonder if we want to just create our own internal abstraction... like a single class that wraps all the JLine stuff, and puts it all in one place, and provides more friendly methods to get to what we need. I'm thinking something along the lines of:
```java
class SomeClassName {
SomeClassName() {
// set up JLine terminal and related objects here
// no JLine types used outside this class
// if we need anything from JLine, we add a convenience method below, so JLine is entirely accessed through this class
}
println(...) { ... }
readline(...) { ... }
saveHistory(...) { ... }
}
var console = new SomeClassName();
console.println("something")
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r566456061
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
Review comment:
> I don't believe this still occurs but I will add it back because I don't have a great way to test and a println doesn't hurt.
Manually/visual testing for this is sufficient.
> I also just noticed that CTRL-D will throw an `EndOfFileException` if I do it on a blank line so I'll add a catch for that exception. This might make that if statement redundant if that is the only situation input could be null.
Hmm, yeah, if it's causing that to explicitly be thrown, then it might be redundant. Handling it by catching the exception is much more clear. As long as the output doesn't get jumbled, and we get a new line when Ctrl-D is entered, I'm not too concerned about how the handling is implemented, as long as the behavior works.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #1824: WIP. Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-754822950
I believe this pull request is ready for review. There are a couple of areas where I unsure of the proper route to go on. One is the use of `DumbTerminal` in [ShellIT](https://github.com/apache/accumulo/blob/61a1755547fc0f9fd6290eb4abe704b775657618/test/src/main/java/org/apache/accumulo/test/ShellIT.java#L166) and [ShellServerIT](https://github.com/Manno15/accumulo/blob/61a1755547fc0f9fd6290eb4abe704b775657618/test/src/main/java/org/apache/accumulo/test/ShellServerIT.java#L198). The other concern is [here](https://github.com/apache/accumulo/blob/61a1755547fc0f9fd6290eb4abe704b775657618/shell/src/main/java/org/apache/accumulo/shell/Shell.java#L563)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565348391
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -182,14 +186,19 @@ public String getErrorMessage() {
public TestOutputStream output;
public StringInputStream input;
public Shell shell;
+ public LineReader reader;
+ public Terminal terminal;
TestShell(String user, String rootPass, String instanceName, String zookeepers, File configFile)
throws IOException {
ClientInfo info = ClientInfo.from(configFile.toPath());
// start the shell
output = new TestOutputStream();
input = new StringInputStream();
- shell = new Shell(new ConsoleReader(input, output));
+ terminal = new DumbTerminal(input, output);
+ terminal.setSize(new Size(80, 24));
+ reader = LineReaderBuilder.builder().terminal(terminal).build();
+ shell = new Shell(reader);
Review comment:
I am unsure if it would change anything since the terminal gets attached to the reader. We could just pass in "terminal" and then build the reader inside `Shell.java` so keep things consistent there. Not sure what that would gain ultimately.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565315656
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -543,23 +551,17 @@ public int start() throws IOException {
if (!accumuloDir.exists() && !accumuloDir.mkdirs()) {
log.warn("Unable to make directory for history at {}", accumuloDir);
}
- try {
- final FileHistory history = new FileHistory(new File(historyPath));
- reader.setHistory(history);
- // Add shutdown hook to flush file history, per jline javadocs
- Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- try {
- history.flush();
- } catch (IOException e) {
- log.warn("Could not flush history to file.");
- }
- }));
- } catch (IOException e) {
- log.warn("Unable to load history file at {}", historyPath);
- }
+
+ // Remove Timestamps for history file. Fixes incompatibility issues
+ reader.unsetOpt(LineReader.Option.HISTORY_TIMESTAMPED);
+
+ // Set history file
+ reader.setVariable(LineReader.HISTORY_FILE, new File(historyPath));
// Turn Ctrl+C into Exception instead of JVM exit
- reader.setHandleUserInterrupt(true);
+ // Not 100% sure this is necessary anymore.
Review comment:
I did find that when I CTRL-C in the middle of the help command, it does work properly. As in it throws the interrupt exception but doesn't exit the process. It only exits if I CTRL-C when just sitting in the shell.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565307321
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -543,23 +551,17 @@ public int start() throws IOException {
if (!accumuloDir.exists() && !accumuloDir.mkdirs()) {
log.warn("Unable to make directory for history at {}", accumuloDir);
}
- try {
- final FileHistory history = new FileHistory(new File(historyPath));
- reader.setHistory(history);
- // Add shutdown hook to flush file history, per jline javadocs
- Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- try {
- history.flush();
- } catch (IOException e) {
- log.warn("Could not flush history to file.");
- }
- }));
- } catch (IOException e) {
- log.warn("Unable to load history file at {}", historyPath);
- }
+
+ // Remove Timestamps for history file. Fixes incompatibility issues
+ reader.unsetOpt(LineReader.Option.HISTORY_TIMESTAMPED);
+
+ // Set history file
+ reader.setVariable(LineReader.HISTORY_FILE, new File(historyPath));
// Turn Ctrl+C into Exception instead of JVM exit
- reader.setHandleUserInterrupt(true);
+ // Not 100% sure this is necessary anymore.
Review comment:
In my testing, both the main branch and these changes do have the same functionality for CTRL-C but they both might be wrong. When I hit CTRL-C, it exits the shell which I do not think is what we are going for. I tested the jline3 demo and CTRL-C will exit back to the shell but not exit the shell. That is probably the functionality we want. The funny thing is, I am implementing what they did in their demo but for some reason, it does not seem to be working. So this will take some more investigating.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1824: WIP. Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-743318211
This is getting close to being finished. Some issues I am facing are history file incompatibilities from previous versions (I believe jline3 requires timestamps in their history file) and hitting `'q'` while in the help command doesn't exit out of the command until you hit `enter` after the `'q'`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565343505
##########
File path: server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java
##########
@@ -271,19 +273,23 @@ static boolean checkInit(VolumeManager fs, SiteConfiguration sconf, Configuratio
return false;
}
if (sconf.get(Property.INSTANCE_SECRET).equals(Property.INSTANCE_SECRET.getDefaultValue())) {
- ConsoleReader c = getConsoleReader();
- c.beep();
- c.println();
- c.println();
- c.println("Warning!!! Your instance secret is still set to the default,"
- + " this is not secure. We highly recommend you change it.");
- c.println();
- c.println();
- c.println("You can change the instance secret in accumulo by using:");
- c.println(" bin/accumulo " + org.apache.accumulo.server.util.ChangeSecret.class.getName());
- c.println("You will also need to edit your secret in your configuration"
- + " file by adding the property instance.secret to your"
- + " accumulo.properties. Without this accumulo will not operate" + " correctly");
+ LineReader c = getLineReader();
+ c.getTerminal().puts(InfoCmp.Capability.bell);
+ c.getTerminal().writer().println();
+ c.getTerminal().writer().println();
+
+ c.getTerminal().writer()
Review comment:
Done in latest commit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r552852154
##########
File path: server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java
##########
@@ -271,19 +273,23 @@ static boolean checkInit(VolumeManager fs, SiteConfiguration sconf, Configuratio
return false;
}
if (sconf.get(Property.INSTANCE_SECRET).equals(Property.INSTANCE_SECRET.getDefaultValue())) {
- ConsoleReader c = getConsoleReader();
- c.beep();
- c.println();
- c.println();
- c.println("Warning!!! Your instance secret is still set to the default,"
- + " this is not secure. We highly recommend you change it.");
- c.println();
- c.println();
- c.println("You can change the instance secret in accumulo by using:");
- c.println(" bin/accumulo " + org.apache.accumulo.server.util.ChangeSecret.class.getName());
- c.println("You will also need to edit your secret in your configuration"
- + " file by adding the property instance.secret to your"
- + " accumulo.properties. Without this accumulo will not operate" + " correctly");
+ LineReader c = getLineReader();
+ c.getTerminal().puts(InfoCmp.Capability.bell);
+ c.getTerminal().writer().println();
Review comment:
Okay great. Thanks for the explanation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565494819
##########
File path: shell/src/test/java/org/apache/accumulo/shell/commands/HistoryCommandTest.java
##########
@@ -56,38 +62,41 @@ public void setUp() throws Exception {
expect(cl.hasOption("np")).andReturn(true);
replay(cl);
- History history = new MemoryHistory();
+ History history = new DefaultHistory();
history.add("foo");
history.add("bar");
baos = new ByteArrayOutputStream();
String input = String.format("!1%n"); // Construct a platform dependent new-line
- reader = new ConsoleReader(new ByteArrayInputStream(input.getBytes()), baos);
- reader.setHistory(history);
+ terminal = TerminalBuilder.builder().system(false)
+ .streams(new ByteArrayInputStream(input.getBytes()), baos).build();
+ reader = LineReaderBuilder.builder().history(history).terminal(terminal).build();
Review comment:
The line reader is what contains the history and also contains the terminal object. Not the other way around. The line reader seems to be the higher-level object though I agree that it doesn't make the most sense. It is probably like this so it doesn't differentiate itself too much from `ConsoleReader` in jline2.
The reader object in terminal is a bit different than a LineReader and isn't as friendly to use.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r566457623
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -182,14 +186,19 @@ public String getErrorMessage() {
public TestOutputStream output;
public StringInputStream input;
public Shell shell;
+ public LineReader reader;
+ public Terminal terminal;
TestShell(String user, String rootPass, String instanceName, String zookeepers, File configFile)
throws IOException {
ClientInfo info = ClientInfo.from(configFile.toPath());
// start the shell
output = new TestOutputStream();
input = new StringInputStream();
- shell = new Shell(new ConsoleReader(input, output));
+ terminal = new DumbTerminal(input, output);
+ terminal.setSize(new Size(80, 24));
+ reader = LineReaderBuilder.builder().terminal(terminal).build();
+ shell = new Shell(reader);
Review comment:
I think your other comment explained that LineReader was more the primary object to interact with, not the terminal. If that's the case, then my suggestion here probably is fine to ignore.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565335477
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
return exitCode;
} // User Canceled (Ctrl+D)
execCommand(input, disableAuthTimeout, false);
} catch (UserInterruptException uie) {
// User Cancelled (Ctrl+C)
- reader.println();
+ writer.println();
String partialLine = uie.getPartialLine();
if (partialLine == null || "".equals(uie.getPartialLine().trim())) {
// No content, actually exit
return exitCode;
}
} finally {
- reader.flush();
+ writer.flush();
}
}
}
public void shutdown() {
if (reader != null) {
- reader.close();
+ try {
+ terminal.close();
+ reader = null;
+ } catch (IOException e) {
+ e.printStackTrace();
Review comment:
I noticed a printExcetion function included in the shell class already so I used that for this. Let me know if that suffices
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565435185
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/HiddenCommand.java
##########
@@ -42,9 +43,9 @@ public String description() {
public int execute(final String fullCommand, final CommandLine cl, final Shell shellState)
throws Exception {
if (rand.nextInt(10) == 0) {
- shellState.getReader().beep();
- shellState.getReader().println();
- shellState.getReader()
+ shellState.getTerminal().puts(InfoCmp.Capability.bell);
Review comment:
The bell should be the same one you hear in Linux if you got backspace repeatedly on an empty line. If you can't hear it by trying that, then it's probably configurable in your system sound settings to enable it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-755590754
Looks good from what I can tell. Manually ran a lot of the commands and everything appears as expected.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1824: WIP. Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-754822950
I believe this pull request is ready for review. There are a couple of areas where I unsure of the proper route to go on. One is the use of `DumbTerminal` in `ShellIT` and `ShellServerIT`. The other is [here](https://github.com/apache/accumulo/blob/61a1755547fc0f9fd6290eb4abe704b775657618/shell/src/main/java/org/apache/accumulo/shell/Shell.java#L563)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565515403
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/HistoryCommand.java
##########
@@ -39,11 +39,11 @@
public int execute(final String fullCommand, final CommandLine cl, final Shell shellState)
throws IOException {
if (cl.hasOption(clearHist.getOpt())) {
- shellState.getReader().getHistory().clear();
+ shellState.getReader().getHistory().purge();
} else {
- Iterator<Entry> source = shellState.getReader().getHistory().entries();
+ Iterator<History.Entry> source = shellState.getReader().getHistory().iterator();
Iterator<String> historyIterator = Iterators.transform(source,
- input -> String.format("%d: %s", input.index() + 1, input.value()));
+ input -> String.format("%d: %s", input.index() + 1, input.line()));
Review comment:
This is something I can look for additional work after this PR. I did briefly look at trying to implement it and noticed a decent amount of code to change at where we interpret the results and print to shell.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565520962
##########
File path: shell/src/main/java/org/apache/accumulo/shell/ShellCompletor.java
##########
@@ -45,13 +48,16 @@ public ShellCompletor(Token rootToken, Map<CompletionSet,Set<String>> options) {
@Override
@SuppressWarnings({"unchecked", "rawtypes"})
Review comment:
Removed in the latest commit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r564800491
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -543,23 +551,17 @@ public int start() throws IOException {
if (!accumuloDir.exists() && !accumuloDir.mkdirs()) {
log.warn("Unable to make directory for history at {}", accumuloDir);
}
- try {
- final FileHistory history = new FileHistory(new File(historyPath));
- reader.setHistory(history);
- // Add shutdown hook to flush file history, per jline javadocs
- Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- try {
- history.flush();
- } catch (IOException e) {
- log.warn("Could not flush history to file.");
- }
- }));
- } catch (IOException e) {
- log.warn("Unable to load history file at {}", historyPath);
- }
+
+ // Remove Timestamps for history file. Fixes incompatibility issues
+ reader.unsetOpt(LineReader.Option.HISTORY_TIMESTAMPED);
+
+ // Set history file
+ reader.setVariable(LineReader.HISTORY_FILE, new File(historyPath));
// Turn Ctrl+C into Exception instead of JVM exit
- reader.setHandleUserInterrupt(true);
+ // Not 100% sure this is necessary anymore.
Review comment:
Did you test CTRL-C without the lines below?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r566765834
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
Review comment:
Another question I have, is if we want to exit on CTRL-D or for CTRL-C for that matter. I am in favor of not exiting on either of those two things but the current implementation is to exit the shell.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-769843956
> @ctubbsii I believe I hit on everyones questions and suggestions. Do you have anything further to add or any concerns?
I can take another look today and re-run the ITs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r552905609
##########
File path: shell/src/main/java/org/apache/accumulo/shell/format/DeleterFormatter.java
##########
@@ -79,33 +79,36 @@ public String next() {
Mutation m = new Mutation(key.getRow());
String entryStr = formatEntry(next, isDoTimestamps());
boolean delete = force;
- try {
- if (!force) {
- shellState.getReader().flush();
+ if (!force) {
+ try {
+ shellState.getWriter().flush();
+ // this will cause end of file Exception for one of the formatter tests
String line = shellState.getReader().readLine("Delete { " + entryStr + " } ? ");
more = line != null;
delete = line != null && (line.equalsIgnoreCase("y") || line.equalsIgnoreCase("yes"));
Review comment:
Could the portion before `&&` be replaced with `more`? I know this is unrelated but thought I would point it out.
```suggestion
delete = more && (line.equalsIgnoreCase("y") || line.equalsIgnoreCase("yes"));
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565480907
##########
File path: test/src/main/java/org/apache/accumulo/test/ShellServerIT.java
##########
@@ -895,7 +904,7 @@ public void clonetableOffline() throws Exception {
ts.exec("scan", false, "TableOfflineException", true);
ts.exec("constraint --list -t " + clone, true, "VisibilityConstraint=2", true);
ts.exec("config -t " + clone + " -np", true, "123M", true);
- ts.exec("getsplits -t " + clone, true, "a\nb\nc\n");
+ ts.exec("getsplits -t " + clone, true, "abc");
Review comment:
Yep, that was my mistake. Apparently, in `PrintShell` I replaced a printLn() with just a print().
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r564800491
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -543,23 +551,17 @@ public int start() throws IOException {
if (!accumuloDir.exists() && !accumuloDir.mkdirs()) {
log.warn("Unable to make directory for history at {}", accumuloDir);
}
- try {
- final FileHistory history = new FileHistory(new File(historyPath));
- reader.setHistory(history);
- // Add shutdown hook to flush file history, per jline javadocs
- Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- try {
- history.flush();
- } catch (IOException e) {
- log.warn("Could not flush history to file.");
- }
- }));
- } catch (IOException e) {
- log.warn("Unable to load history file at {}", historyPath);
- }
+
+ // Remove Timestamps for history file. Fixes incompatibility issues
+ reader.unsetOpt(LineReader.Option.HISTORY_TIMESTAMPED);
+
+ // Set history file
+ reader.setVariable(LineReader.HISTORY_FILE, new File(historyPath));
// Turn Ctrl+C into Exception instead of JVM exit
- reader.setHandleUserInterrupt(true);
+ // Not 100% sure this is necessary anymore.
Review comment:
Did you test CTRL-C without the lines below? Or `kill -2 <pid>` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565533879
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
Review comment:
I don't believe this still occurs but I will add it back because I don't have a great way to test and a println doesn't hurt. I also just noticed that CTRL-D will throw an `EndOfFileException` if I do it on a blank line so I'll add a catch for that exception. This might make that if statement redundant.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565350686
##########
File path: shell/src/test/java/org/apache/accumulo/shell/format/DeleterFormatterTest.java
##########
@@ -133,6 +141,7 @@ public void testNo() throws IOException {
assertTrue(formatter.hasNext());
}
+ // Need to properly fix this test.
Review comment:
Related to your other comment below in `DeleterFormatter`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565358488
##########
File path: shell/src/main/java/org/apache/accumulo/shell/format/DeleterFormatter.java
##########
@@ -79,33 +79,36 @@ public String next() {
Mutation m = new Mutation(key.getRow());
String entryStr = formatEntry(next, isDoTimestamps());
boolean delete = force;
- try {
- if (!force) {
- shellState.getReader().flush();
+ if (!force) {
+ try {
+ shellState.getWriter().flush();
+ // this will cause end of file Exception for one of the formatter tests
Review comment:
the failure happens on line 86 where it tries to read the delete prompt. When `DeleterFormatterTest.testNoConfirmation()` is ran, it hits that line but gets an end of file marker instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565307321
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -543,23 +551,17 @@ public int start() throws IOException {
if (!accumuloDir.exists() && !accumuloDir.mkdirs()) {
log.warn("Unable to make directory for history at {}", accumuloDir);
}
- try {
- final FileHistory history = new FileHistory(new File(historyPath));
- reader.setHistory(history);
- // Add shutdown hook to flush file history, per jline javadocs
- Runtime.getRuntime().addShutdownHook(new Thread(() -> {
- try {
- history.flush();
- } catch (IOException e) {
- log.warn("Could not flush history to file.");
- }
- }));
- } catch (IOException e) {
- log.warn("Unable to load history file at {}", historyPath);
- }
+
+ // Remove Timestamps for history file. Fixes incompatibility issues
+ reader.unsetOpt(LineReader.Option.HISTORY_TIMESTAMPED);
+
+ // Set history file
+ reader.setVariable(LineReader.HISTORY_FILE, new File(historyPath));
// Turn Ctrl+C into Exception instead of JVM exit
- reader.setHandleUserInterrupt(true);
+ // Not 100% sure this is necessary anymore.
Review comment:
In my testing, both the main branch and these changes do have the same functionality for CTRL-C but they both might be wrong. When I hit CTRL-C, it exits the shell which I do not think is what we are going for. I tested the jline3 demo and CTRL-C will reset back to the shell but not exit the shell. That is probably the functionality we want. The funny thing is, I am implementing what they did in their demo but for some reason, it does not seem to be working. So this will take some more investigating.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r566744725
##########
File path: shell/src/main/java/org/apache/accumulo/shell/format/DeleterFormatter.java
##########
@@ -79,33 +79,36 @@ public String next() {
Mutation m = new Mutation(key.getRow());
String entryStr = formatEntry(next, isDoTimestamps());
boolean delete = force;
- try {
- if (!force) {
- shellState.getReader().flush();
+ if (!force) {
+ try {
+ shellState.getWriter().flush();
+ // this will cause end of file Exception for one of the formatter tests
String line = shellState.getReader().readLine("Delete { " + entryStr + " } ? ");
more = line != null;
delete = line != null && (line.equalsIgnoreCase("y") || line.equalsIgnoreCase("yes"));
Review comment:
I will add this. Sorry for the overlook.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1824: WIP. Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#issuecomment-739844395
Yeah, that sounds like a great idea @ctubbsii. I will look into to implementing something like that while I continue to fix some of the areas I was unsure on.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565350356
##########
File path: shell/src/main/java/org/apache/accumulo/shell/format/DeleterFormatter.java
##########
@@ -79,33 +79,36 @@ public String next() {
Mutation m = new Mutation(key.getRow());
String entryStr = formatEntry(next, isDoTimestamps());
boolean delete = force;
- try {
- if (!force) {
- shellState.getReader().flush();
+ if (!force) {
+ try {
+ shellState.getWriter().flush();
+ // this will cause end of file Exception for one of the formatter tests
Review comment:
This is related to the other comment you mentioned in the `DeleterFormatterTest`. I will have to refresh my memory on it a bit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r565408422
##########
File path: shell/src/main/java/org/apache/accumulo/shell/format/DeleterFormatter.java
##########
@@ -79,33 +79,36 @@ public String next() {
Mutation m = new Mutation(key.getRow());
String entryStr = formatEntry(next, isDoTimestamps());
boolean delete = force;
- try {
- if (!force) {
- shellState.getReader().flush();
+ if (!force) {
+ try {
+ shellState.getWriter().flush();
+ // this will cause end of file Exception for one of the formatter tests
Review comment:
It is possible that Jline3 changed something behind the scenes. On the main branch, line returns null when trying to read the prompt. Currently, it reads the end of file. Not sure what the best approach to fix it yet but I am trying to get it to where it returns null.
EDIT: Looks like the EndOfFile Exception is new to jline3 and is thrown by readLine now. I should have noticed that difference earlier. Not sure the best way to adapt `DeleterFormatter` to take that into consideration.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1824: Upgrade to jline3
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1824:
URL: https://github.com/apache/accumulo/pull/1824#discussion_r566765834
##########
File path: shell/src/main/java/org/apache/accumulo/shell/Shell.java
##########
@@ -584,40 +586,39 @@ public int start() throws IOException {
// If tab completion is true we need to reset
if (tabCompletion) {
- if (userCompletor != null) {
- reader.removeCompleter(userCompletor);
- }
-
userCompletor = setupCompletion();
- reader.addCompleter(userCompletor);
+ ((LineReaderImpl) reader).setCompleter(userCompletor);
}
- reader.setPrompt(getDefaultPrompt());
- input = reader.readLine();
+ input = reader.readLine(getDefaultPrompt());
if (input == null) {
- reader.println();
Review comment:
Another question I have, is if we want to exit on CTRL-D or for CTRL-C for that matter (exits only when just sitting in the terminal). I am in favor of not exiting on either of those two things but the current implementation is to exit the shell.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org