You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by mikewalch <gi...@git.apache.org> on 2017/03/20 17:23:04 UTC
[GitHub] accumulo pull request #233: ACCUMULO-4604 Improve 'accumulo classpath' comma...
GitHub user mikewalch opened a pull request:
https://github.com/apache/accumulo/pull/233
ACCUMULO-4604 Improve 'accumulo classpath' command
* Default output is now colon seperated jars on one line
* Added '-d' option to print old human readable format
* Improved usage for accumulo script commands
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mikewalch/accumulo classpath-cmd
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/233.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #233
----
commit 3a3df0c9a1f1075b543e8794b1ee889be021ffc1
Author: Mike Walch <mw...@apache.org>
Date: 2017-03-10T20:27:21Z
ACCUMULO-4604 Improve 'accumulo classpath' command
* Default output is now colon seperated jars on one line
* Added '-d' option to print old human readable format
* Improved usage for accumulo script commands
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #233: ACCUMULO-4604 Improve 'accumulo classpath' comma...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/233#discussion_r106967673
--- Diff: start/src/test/java/org/apache/accumulo/start/classloader/vfs/AccumuloClasspathTest.java ---
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.start.classloader.vfs;
+
+import static org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassPath;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class AccumuloClasspathTest {
+
+ @Test
+ public void basic() {
+ Assert.assertTrue(getClassPath(true).contains("\t"));
+ Assert.assertTrue(getClassPath(true).contains("Java System Classloader"));
+ Assert.assertTrue(getClassPath(true).contains("Level"));
+ Assert.assertTrue(getClassPath(true).length() > getClassPath(false).length());
+ Assert.assertFalse(getClassPath(false).contains("\t"));
--- End diff --
Would it make sense to ensure there is no white space?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #233: ACCUMULO-4604 Improve 'accumulo classpath' comma...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/233#discussion_r106990071
--- Diff: start/src/test/java/org/apache/accumulo/start/classloader/vfs/AccumuloClasspathTest.java ---
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.start.classloader.vfs;
+
+import static org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.getClassPath;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class AccumuloClasspathTest {
+
+ @Test
+ public void basic() {
+ Assert.assertTrue(getClassPath(true).contains("\t"));
+ Assert.assertTrue(getClassPath(true).contains("Java System Classloader"));
+ Assert.assertTrue(getClassPath(true).contains("Level"));
+ Assert.assertTrue(getClassPath(true).length() > getClassPath(false).length());
+ Assert.assertFalse(getClassPath(false).contains("\t"));
--- End diff --
Added check in 83ae69fea38
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #233: ACCUMULO-4604 Improve 'accumulo classpath' comma...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/233#discussion_r106967852
--- Diff: core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java ---
@@ -121,7 +121,7 @@ public String description() {
@Override
public void execute(final String[] args) throws Exception {
Opts opts = new Opts();
- opts.parseArgs(PrintInfo.class.getName(), args);
+ opts.parseArgs("accumulo rfile-info", args);
--- End diff --
thats nice
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #233: ACCUMULO-4604 Improve 'accumulo classpath' comma...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/233
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #233: ACCUMULO-4604 Improve 'accumulo classpath' command
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on the issue:
https://github.com/apache/accumulo/pull/233
Executing the following reveals a lot of duplicates out of the box. This should be resolved with your other PR to trim out the redundantly configured jars from the site file.
```bash
accumulo classpath | tr ':' "\n" | less | sort | uniq -c | grep -v ' 1 '
```
To test that, I set `general.classpaths` to `/var/tmp/` in `accumulo-site.xml`, and re-ran this command. I verified that it removed the duplicates, but it also added an item, `$(pwd)//var/tmp/`, to the output. I set it to empty string, and it added `$(pwd)/` to the output. These are both incorrect. The current directory should never get added, and absolute paths should not be resolved relative to the current directory.
Also, it's not clear that the expected output of this command should include stuff from the `AccumuloClassLoader` or the `AccumuloVFSClassLoader` by default (without specifying some sort of depth/level).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #233: ACCUMULO-4604 Improve 'accumulo classpath' comma...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/233#discussion_r107795526
--- Diff: start/src/main/java/org/apache/accumulo/start/classloader/vfs/AccumuloVFSClassLoader.java ---
@@ -283,16 +283,27 @@ private static File computeTopCacheDir() {
void print(String s);
}
- public static void printClassPath() {
- printClassPath(new Printer() {
- @Override
- public void print(String s) {
- System.out.println(s);
- }
- });
+ public static void printClassPath(boolean debug) {
+ printClassPath(System.out::print, debug);
+ }
+
+ public static String getClassPath(boolean debug) {
+ StringBuilder cp = new StringBuilder();
+ printClassPath(s -> cp.append(s), debug);
+ return cp.toString();
}
- public static void printClassPath(Printer out) {
+ private static void printJar(Printer out, String jarPath, boolean debug, boolean sawFirst) {
--- End diff --
Have you verified that this still prints directories on the class path correctly? Also, do we want this class to always print the full classpath, including what's on the VFS classloader path (which might have URLs not parse-able by other class loaders). Might want to have an option for depth, with the default just being the value of `System.getProperty("java.class.path")`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #233: ACCUMULO-4604 Improve 'accumulo classpath' command
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on the issue:
https://github.com/apache/accumulo/pull/233
I am going to merge today if there are no objections
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #233: ACCUMULO-4604 Improve 'accumulo classpath' comma...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/233#discussion_r107201757
--- Diff: core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java ---
@@ -121,7 +121,7 @@ public String description() {
@Override
public void execute(final String[] args) throws Exception {
Opts opts = new Opts();
- opts.parseArgs(PrintInfo.class.getName(), args);
+ opts.parseArgs("accumulo rfile-info", args);
--- End diff --
How did this work before? I have used just "accumulo rifile-info <rfile>" without the classname...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #233: ACCUMULO-4604 Improve 'accumulo classpath' comma...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/233#discussion_r107207736
--- Diff: core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java ---
@@ -121,7 +121,7 @@ public String description() {
@Override
public void execute(final String[] args) throws Exception {
Opts opts = new Opts();
- opts.parseArgs(PrintInfo.class.getName(), args);
+ opts.parseArgs("accumulo rfile-info", args);
--- End diff --
I am just changing the usage. Previously, it would print:
```bash
accumulo rfile-info -h
Usage: org.apache.accumulo.core.file.rfile.PrintInfo [options] <file> { <file> ... }
Options:
-c, --config
Comma-separated Hadoop configuration files
Default: []
```
It now prints:
```bash
accumulo rfile-info -h
Usage: accumulo rfile-info [options] <file> { <file> ... }
Options:
-c, --config
Comma-separated Hadoop configuration files
Default: []
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #233: ACCUMULO-4604 Improve 'accumulo classpath' comma...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/233#discussion_r107794051
--- Diff: core/src/main/java/org/apache/accumulo/core/util/Classpath.java ---
@@ -16,13 +16,21 @@
*/
package org.apache.accumulo.core.util;
+import com.beust.jcommander.Parameter;
import org.apache.accumulo.start.Main;
import org.apache.accumulo.start.spi.KeywordExecutable;
import com.google.auto.service.AutoService;
@AutoService(KeywordExecutable.class)
public class Classpath implements KeywordExecutable {
+
+ static class Opts extends org.apache.accumulo.core.cli.Help {
+
+ @Parameter(names = {"-d", "--debug"}, description = "Turns on debugging")
--- End diff --
Is this actually debugging, or verbosity?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #233: ACCUMULO-4604 Improve 'accumulo classpath' comma...
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/233#discussion_r106968119
--- Diff: shell/src/main/java/org/apache/accumulo/shell/commands/ClasspathCommand.java ---
@@ -23,23 +23,19 @@
import org.apache.accumulo.shell.Shell;
import org.apache.accumulo.shell.Shell.Command;
import org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader;
-import org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader.Printer;
import org.apache.commons.cli.CommandLine;
public class ClasspathCommand extends Command {
@Override
public int execute(final String fullCommand, final CommandLine cl, final Shell shellState) {
final ConsoleReader reader = shellState.getReader();
- AccumuloVFSClassLoader.printClassPath(new Printer() {
- @Override
- public void print(String s) {
- try {
- reader.println(s);
- } catch (IOException ex) {
- throw new RuntimeException(ex);
- }
+ AccumuloVFSClassLoader.printClassPath(s -> {
+ try {
+ reader.println(s);
+ } catch (IOException ex) {
+ throw new RuntimeException(ex);
--- End diff --
there is the new UncheckedIOException
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #233: ACCUMULO-4604 Improve 'accumulo classpath' comma...
Posted by milleruntime <gi...@git.apache.org>.
Github user milleruntime commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/233#discussion_r107210730
--- Diff: core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java ---
@@ -121,7 +121,7 @@ public String description() {
@Override
public void execute(final String[] args) throws Exception {
Opts opts = new Opts();
- opts.parseArgs(PrintInfo.class.getName(), args);
+ opts.parseArgs("accumulo rfile-info", args);
--- End diff --
Ahhhhh gotcha. Much better!!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---