You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by dlmarion <gi...@git.apache.org> on 2016/06/08 19:10:13 UTC
[GitHub] accumulo pull request #111: ACCUMULO-4331: first draft
GitHub user dlmarion opened a pull request:
https://github.com/apache/accumulo/pull/111
ACCUMULO-4331: first draft
First draft. Did not implement the range syntax that was proposed in later comments, but would be easy to swap out. There are some ports where it seemed not to make sense to support a range, log4j monitor port being one as its also set as a system property.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dlmarion/accumulo ACCUMULO-4331
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/111.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 #111
----
commit c726613ae2ed3e94ac6071bd334c8ee64ac9b26e
Author: Dave Marion <dl...@apache.org>
Date: 2016-06-08T19:07:21Z
ACCUMULO-4331: first draft
----
---
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 #111: ACCUMULO-4331: first draft
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r67005900
--- Diff: server/base/src/main/java/org/apache/accumulo/server/util/Admin.java ---
@@ -374,15 +374,17 @@ private static void stopTabletServer(final ClientContext context, List<String> s
final String zTServerRoot = getTServersZkPath(instance);
final ZooCache zc = new ZooCacheFactory().getZooCache(instance.getZooKeepers(), instance.getZooKeepersSessionTimeOut());
for (String server : servers) {
- HostAndPort address = AddressUtil.parseAddress(server, context.getConfiguration().getPort(Property.TSERV_CLIENTPORT));
- final String finalServer = qualifyWithZooKeeperSessionId(zTServerRoot, zc, address.toString());
- log.info("Stopping server " + finalServer);
- MasterClient.execute(context, new ClientExec<MasterClientService.Client>() {
- @Override
- public void execute(MasterClientService.Client client) throws Exception {
- client.shutdownTabletServer(Tracer.traceInfo(), context.rpcCreds(), finalServer, force);
- }
- });
+ for (int port : context.getConfiguration().getPort(Property.TSERV_CLIENTPORT)) {
--- End diff --
Filed https://issues.apache.org/jira/browse/ACCUMULO-4342 for this
---
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 #111: ACCUMULO-4331: first draft
Posted by ShawnWalker <gi...@git.apache.org>.
Github user ShawnWalker commented on the issue:
https://github.com/apache/accumulo/pull/111
Maybe integrate the build-helper-maven-plugin to reserve ephemeral ports? http://www.mojohaus.org/build-helper-maven-plugin/reserve-network-port-mojo.html
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on the issue:
https://github.com/apache/accumulo/pull/111
FWIW, with this latest change I was able to start Accumulo to include two tablet servers with setting all of the *.port.client properties to the range of 11000-11006.
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r67005411
--- Diff: core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java ---
@@ -72,4 +72,79 @@ public void testGetPropertyByString() {
}
assertTrue("test was a dud, and did nothing", found);
}
+
+ @Test
+ public void testGetSinglePort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(9997, ports[0]);
+ }
+
+ @Test
+ public void testGetAnyPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "0");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(0, ports[0]);
+ }
+
+ @Test
+ public void testGetInvalidPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(Integer.parseInt(Property.TSERV_CLIENTPORT.getDefaultValue()), ports[0]);
+ }
+
+ @Test
+ public void testGetPortRange() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997-9999");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(9997, ports[0]);
+ assertEquals(9998, ports[1]);
+ assertEquals(9999, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidLow() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020-1026");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(1024, ports[0]);
+ assertEquals(1025, ports[1]);
+ assertEquals(1026, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidHigh() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "65533-65538");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(65533, ports[0]);
+ assertEquals(65534, ports[1]);
+ assertEquals(65535, ports[2]);
+ }
+
+ @Test(expected = IllegalArgumentException.class)
--- End diff --
bq. More over, why are we supporting multiple versions of "range" syntax in the first place?
We don't. See my earlier unhelpful comment. Only M-N is supported. An IllegalArgumentException is thrown because of the invalid range syntax, not because of the out of range port.
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r67002968
--- Diff: core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java ---
@@ -72,4 +72,79 @@ public void testGetPropertyByString() {
}
assertTrue("test was a dud, and did nothing", found);
}
+
+ @Test
+ public void testGetSinglePort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(9997, ports[0]);
+ }
+
+ @Test
+ public void testGetAnyPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "0");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(0, ports[0]);
+ }
+
+ @Test
+ public void testGetInvalidPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(Integer.parseInt(Property.TSERV_CLIENTPORT.getDefaultValue()), ports[0]);
+ }
+
+ @Test
+ public void testGetPortRange() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997-9999");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(9997, ports[0]);
+ assertEquals(9998, ports[1]);
+ assertEquals(9999, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidLow() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020-1026");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(1024, ports[0]);
+ assertEquals(1025, ports[1]);
+ assertEquals(1026, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidHigh() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "65533-65538");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(65533, ports[0]);
+ assertEquals(65534, ports[1]);
+ assertEquals(65535, ports[2]);
+ }
+
+ @Test(expected = IllegalArgumentException.class)
--- End diff --
Because the range syntax is M-N, not [M,N].
---
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 #111: ACCUMULO-4331: first draft
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/111#discussion_r66349478
--- Diff: core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java ---
@@ -56,8 +56,10 @@
+ "Examples of valid host lists are 'localhost:2000,www.example.com,10.10.1.1:500' and 'localhost'.\n"
+ "Examples of invalid host lists are '', ':1000', and 'localhost:80000'"),
- PORT("port", Predicates.or(new Bounds(1024, 65535), in(true, "0")),
- "An positive integer in the range 1024-65535, not already in use or specified elsewhere in the configuration"),
+ @SuppressWarnings("unchecked")
+ PORT("port", Predicates.or(new Bounds(1024, 65535), in(true, "0"), new Matches("\\d{1,5}-\\d{1,5}")),
--- End diff --
The regex could be a bit more restrictive, like `\\d{4,5}-\\d{4,5}`
Would there be a benefit for creating a Predicate to verify the port range, like maybe fail sooner (when setting a prop in shell). Instead of validating in getPorts().
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r66501071
--- Diff: server/base/src/main/java/org/apache/accumulo/server/Accumulo.java ---
@@ -168,8 +168,8 @@ public static void init(VolumeManager fs, ServerConfigurationFactory serverConfi
logConfigWatcher.start();
// Makes sure the log-forwarding to the monitor is configured
- int logPort = conf.getPort(Property.MONITOR_LOG4J_PORT);
- System.setProperty("org.apache.accumulo.core.host.log.port", Integer.toString(logPort));
+ int logPort[] = conf.getPort(Property.MONITOR_LOG4J_PORT);
--- End diff --
Doh! Good catch, didn't intend to do that
---
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 #111: ACCUMULO-4331: first draft
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/111#discussion_r67011931
--- Diff: core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java ---
@@ -262,4 +267,46 @@ public boolean apply(final String input) {
}
+ public static class PortRange extends Matches {
+
+ private static final Logger log = LoggerFactory.getLogger(PortRange.class);
+
+ public PortRange(final String pattern) {
+ super(pattern);
+ }
+
+ @Override
+ public boolean apply(final String input) {
+ if (super.apply(input)) {
+ try {
+ PortRange.parse(input);
+ return true;
+ } catch (IllegalArgumentException e) {
+ return false;
+ }
+ } else {
+ return false;
+ }
+ }
+
+ public static Pair<Integer,Integer> parse(String portRange) {
+ int idx = portRange.indexOf('-');
+ if (idx != -1) {
+ int low = Integer.parseInt(portRange.substring(0, idx));
+ if (low < 1024) {
--- End diff --
> These checks were not there previously
Previously we didn't have ranges?
> This PR is already closed and the code committed.
Ok
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on the issue:
https://github.com/apache/accumulo/pull/111
Will apply manually.
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on the issue:
https://github.com/apache/accumulo/pull/111
There are no tests for TServerUtils.startServer. I think I can create some, the problem is that I don't know what ephemeral ports are open on any given test machine. Ideas?
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion closed the pull request at:
https://github.com/apache/accumulo/pull/111
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r67013512
--- Diff: core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java ---
@@ -262,4 +267,46 @@ public boolean apply(final String input) {
}
+ public static class PortRange extends Matches {
+
+ private static final Logger log = LoggerFactory.getLogger(PortRange.class);
+
+ public PortRange(final String pattern) {
+ super(pattern);
+ }
+
+ @Override
+ public boolean apply(final String input) {
+ if (super.apply(input)) {
+ try {
+ PortRange.parse(input);
+ return true;
+ } catch (IllegalArgumentException e) {
+ return false;
+ }
+ } else {
+ return false;
+ }
+ }
+
+ public static Pair<Integer,Integer> parse(String portRange) {
+ int idx = portRange.indexOf('-');
+ if (idx != -1) {
+ int low = Integer.parseInt(portRange.substring(0, idx));
+ if (low < 1024) {
--- End diff --
Looks like I missed a range check:
if (port < 1024 || port > 65535) {
log.error("Invalid port number " + port + "; Using default " + property.getDefaultValue());
I'll create a JIRA issue for it.
---
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 #111: ACCUMULO-4331: first draft
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the issue:
https://github.com/apache/accumulo/pull/111
Being lazy, I suspect the following is the behavior but not 100% sure. @dlmarion hoping you might know off the top of your head since you have been working this. If not I can research it more.
* If I set a port to 100000 in the shell, it will fail and not change the port setting.
* If I set a port to 5000-7000 in the shell, it will log an error in the shell and set the port to 5000-64K
If this is the behavior, then it seems inconsistent to me.
---
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 #111: ACCUMULO-4331: first draft
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r67006266
--- Diff: core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java ---
@@ -72,4 +72,79 @@ public void testGetPropertyByString() {
}
assertTrue("test was a dud, and did nothing", found);
}
+
+ @Test
+ public void testGetSinglePort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(9997, ports[0]);
+ }
+
+ @Test
+ public void testGetAnyPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "0");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(0, ports[0]);
+ }
+
+ @Test
+ public void testGetInvalidPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(Integer.parseInt(Property.TSERV_CLIENTPORT.getDefaultValue()), ports[0]);
+ }
+
+ @Test
+ public void testGetPortRange() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997-9999");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(9997, ports[0]);
+ assertEquals(9998, ports[1]);
+ assertEquals(9999, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidLow() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020-1026");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(1024, ports[0]);
+ assertEquals(1025, ports[1]);
+ assertEquals(1026, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidHigh() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "65533-65538");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(65533, ports[0]);
+ assertEquals(65534, ports[1]);
+ assertEquals(65535, ports[2]);
+ }
+
+ @Test(expected = IllegalArgumentException.class)
--- End diff --
Oh. I didn't realize you didn't actually follow the suggestion to use standard math range notations. That would explain my confusion.
---
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 #111: ACCUMULO-4331: first draft
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r67003707
--- Diff: core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java ---
@@ -72,4 +72,79 @@ public void testGetPropertyByString() {
}
assertTrue("test was a dud, and did nothing", found);
}
+
+ @Test
+ public void testGetSinglePort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(9997, ports[0]);
+ }
+
+ @Test
+ public void testGetAnyPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "0");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(0, ports[0]);
+ }
+
+ @Test
+ public void testGetInvalidPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(Integer.parseInt(Property.TSERV_CLIENTPORT.getDefaultValue()), ports[0]);
+ }
+
+ @Test
+ public void testGetPortRange() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997-9999");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(9997, ports[0]);
+ assertEquals(9998, ports[1]);
+ assertEquals(9999, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidLow() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020-1026");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(1024, ports[0]);
+ assertEquals(1025, ports[1]);
+ assertEquals(1026, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidHigh() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "65533-65538");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(65533, ports[0]);
+ assertEquals(65534, ports[1]);
+ assertEquals(65535, ports[2]);
+ }
+
+ @Test(expected = IllegalArgumentException.class)
--- End diff --
That's a super unhelpful answer. Obviously I can see the syntax is different. I'm asking *why* the semantics are different in error handling for the different syntax.
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on the issue:
https://github.com/apache/accumulo/pull/111
Added tests to TServerUtils. For some reason the MiniAccumuloCluster tests are failing for me now. Not sure how it is related yet.
---
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 #111: ACCUMULO-4331: first draft
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/111#discussion_r67010288
--- Diff: core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java ---
@@ -262,4 +267,46 @@ public boolean apply(final String input) {
}
+ public static class PortRange extends Matches {
+
+ private static final Logger log = LoggerFactory.getLogger(PortRange.class);
+
+ public PortRange(final String pattern) {
+ super(pattern);
+ }
+
+ @Override
+ public boolean apply(final String input) {
+ if (super.apply(input)) {
+ try {
+ PortRange.parse(input);
+ return true;
+ } catch (IllegalArgumentException e) {
+ return false;
+ }
+ } else {
+ return false;
+ }
+ }
+
+ public static Pair<Integer,Integer> parse(String portRange) {
+ int idx = portRange.indexOf('-');
+ if (idx != -1) {
+ int low = Integer.parseInt(portRange.substring(0, idx));
+ if (low < 1024) {
+ log.error("Invalid port number for low end of the range, using 1024");
+ low = 1024;
+ }
+ int high = Integer.parseInt(portRange.substring(idx + 1));
--- End diff --
or maybe `<=`
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r67003317
--- Diff: server/base/src/main/java/org/apache/accumulo/server/util/Admin.java ---
@@ -374,15 +374,17 @@ private static void stopTabletServer(final ClientContext context, List<String> s
final String zTServerRoot = getTServersZkPath(instance);
final ZooCache zc = new ZooCacheFactory().getZooCache(instance.getZooKeepers(), instance.getZooKeepersSessionTimeOut());
for (String server : servers) {
- HostAndPort address = AddressUtil.parseAddress(server, context.getConfiguration().getPort(Property.TSERV_CLIENTPORT));
- final String finalServer = qualifyWithZooKeeperSessionId(zTServerRoot, zc, address.toString());
- log.info("Stopping server " + finalServer);
- MasterClient.execute(context, new ClientExec<MasterClientService.Client>() {
- @Override
- public void execute(MasterClientService.Client client) throws Exception {
- client.shutdownTabletServer(Tracer.traceInfo(), context.rpcCreds(), finalServer, force);
- }
- });
+ for (int port : context.getConfiguration().getPort(Property.TSERV_CLIENTPORT)) {
--- End diff --
Good catch, I wonder how long that has been around.
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r67008218
--- Diff: core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java ---
@@ -72,4 +72,79 @@ public void testGetPropertyByString() {
}
assertTrue("test was a dud, and did nothing", found);
}
+
+ @Test
+ public void testGetSinglePort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(9997, ports[0]);
+ }
+
+ @Test
+ public void testGetAnyPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "0");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(0, ports[0]);
+ }
+
+ @Test
+ public void testGetInvalidPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(Integer.parseInt(Property.TSERV_CLIENTPORT.getDefaultValue()), ports[0]);
+ }
+
+ @Test
+ public void testGetPortRange() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997-9999");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(9997, ports[0]);
+ assertEquals(9998, ports[1]);
+ assertEquals(9999, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidLow() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020-1026");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(1024, ports[0]);
+ assertEquals(1025, ports[1]);
+ assertEquals(1026, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidHigh() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "65533-65538");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(65533, ports[0]);
+ assertEquals(65534, ports[1]);
+ assertEquals(65535, ports[2]);
+ }
+
+ @Test(expected = IllegalArgumentException.class)
--- End diff --
If someone decides that they want to change the syntax, they just need to change the PortRange.parse method.
My decision was based on the fact that neither Keith or I could find a library for parsing the proposed syntax, and I think the syntax may actually be more confusing to the end user.
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on the issue:
https://github.com/apache/accumulo/pull/111
Tested locally with two tservers and port range appears to be working.
---
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 #111: ACCUMULO-4331: first draft
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r66497618
--- Diff: server/base/src/main/java/org/apache/accumulo/server/Accumulo.java ---
@@ -168,8 +168,8 @@ public static void init(VolumeManager fs, ServerConfigurationFactory serverConfi
logConfigWatcher.start();
// Makes sure the log-forwarding to the monitor is configured
- int logPort = conf.getPort(Property.MONITOR_LOG4J_PORT);
- System.setProperty("org.apache.accumulo.core.host.log.port", Integer.toString(logPort));
+ int logPort[] = conf.getPort(Property.MONITOR_LOG4J_PORT);
--- End diff --
Can you put brackets after the type instead of the variable? This is form is valid but [discouraged](https://docs.oracle.com/javase/tutorial/java/nutsandbolts/arrays.html). This occurs a couple different times in the PR.
---
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 #111: ACCUMULO-4331: first draft
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r67007078
--- Diff: core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java ---
@@ -72,4 +72,79 @@ public void testGetPropertyByString() {
}
assertTrue("test was a dud, and did nothing", found);
}
+
+ @Test
+ public void testGetSinglePort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(9997, ports[0]);
+ }
+
+ @Test
+ public void testGetAnyPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "0");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(0, ports[0]);
+ }
+
+ @Test
+ public void testGetInvalidPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(Integer.parseInt(Property.TSERV_CLIENTPORT.getDefaultValue()), ports[0]);
+ }
+
+ @Test
+ public void testGetPortRange() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997-9999");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(9997, ports[0]);
+ assertEquals(9998, ports[1]);
+ assertEquals(9999, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidLow() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020-1026");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(1024, ports[0]);
+ assertEquals(1025, ports[1]);
+ assertEquals(1026, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidHigh() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "65533-65538");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(65533, ports[0]);
+ assertEquals(65534, ports[1]);
+ assertEquals(65535, ports[2]);
+ }
+
+ @Test(expected = IllegalArgumentException.class)
--- End diff --
Yeah, sorry. I didn't look closely enough at the changeset you committed after the last review. I had just assumed that since Keith and I had agreed that would be a good idea that you had done that. That, tied in with a test case that used that syntax but with invalid bounds further cemented the assumption.
Anyways, sorry for the confusion.
---
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 #111: ACCUMULO-4331: first draft
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r67004643
--- Diff: core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java ---
@@ -72,4 +72,79 @@ public void testGetPropertyByString() {
}
assertTrue("test was a dud, and did nothing", found);
}
+
+ @Test
+ public void testGetSinglePort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(9997, ports[0]);
+ }
+
+ @Test
+ public void testGetAnyPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "0");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(0, ports[0]);
+ }
+
+ @Test
+ public void testGetInvalidPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(Integer.parseInt(Property.TSERV_CLIENTPORT.getDefaultValue()), ports[0]);
+ }
+
+ @Test
+ public void testGetPortRange() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997-9999");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(9997, ports[0]);
+ assertEquals(9998, ports[1]);
+ assertEquals(9999, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidLow() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020-1026");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(1024, ports[0]);
+ assertEquals(1025, ports[1]);
+ assertEquals(1026, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidHigh() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "65533-65538");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(65533, ports[0]);
+ assertEquals(65534, ports[1]);
+ assertEquals(65535, ports[2]);
+ }
+
+ @Test(expected = IllegalArgumentException.class)
--- End diff --
> testGetPortRangeInvalidHigh does not throw an error
Right, but `testGetPortInvalidSyntax` does through an error when the upper bound of the range is outside of the acceptable range.
> It truncates the range to a valid set of port numbers.
Exactly: why does one "range" syntax automatically truncate configuration values to a valid range while another "range" syntax fail?
More over, why are we supporting multiple version of "range" syntax in the first place?
---
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 #111: ACCUMULO-4331: first draft
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/111#discussion_r67009954
--- Diff: core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java ---
@@ -262,4 +267,46 @@ public boolean apply(final String input) {
}
+ public static class PortRange extends Matches {
+
+ private static final Logger log = LoggerFactory.getLogger(PortRange.class);
+
+ public PortRange(final String pattern) {
+ super(pattern);
+ }
+
+ @Override
+ public boolean apply(final String input) {
+ if (super.apply(input)) {
+ try {
+ PortRange.parse(input);
+ return true;
+ } catch (IllegalArgumentException e) {
+ return false;
+ }
+ } else {
+ return false;
+ }
+ }
+
+ public static Pair<Integer,Integer> parse(String portRange) {
+ int idx = portRange.indexOf('-');
+ if (idx != -1) {
+ int low = Integer.parseInt(portRange.substring(0, idx));
+ if (low < 1024) {
--- End diff --
what about case where `high < 1024`
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r67011320
--- Diff: core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java ---
@@ -262,4 +267,46 @@ public boolean apply(final String input) {
}
+ public static class PortRange extends Matches {
+
+ private static final Logger log = LoggerFactory.getLogger(PortRange.class);
+
+ public PortRange(final String pattern) {
+ super(pattern);
+ }
+
+ @Override
+ public boolean apply(final String input) {
+ if (super.apply(input)) {
+ try {
+ PortRange.parse(input);
+ return true;
+ } catch (IllegalArgumentException e) {
+ return false;
+ }
+ } else {
+ return false;
+ }
+ }
+
+ public static Pair<Integer,Integer> parse(String portRange) {
+ int idx = portRange.indexOf('-');
+ if (idx != -1) {
+ int low = Integer.parseInt(portRange.substring(0, idx));
+ if (low < 1024) {
--- End diff --
These checks were not there previously. This PR is already closed and the code committed. You will have to open up tickets for these new issues.
---
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 #111: ACCUMULO-4331: first draft
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/111#discussion_r67010037
--- Diff: core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java ---
@@ -262,4 +267,46 @@ public boolean apply(final String input) {
}
+ public static class PortRange extends Matches {
+
+ private static final Logger log = LoggerFactory.getLogger(PortRange.class);
+
+ public PortRange(final String pattern) {
+ super(pattern);
+ }
+
+ @Override
+ public boolean apply(final String input) {
+ if (super.apply(input)) {
+ try {
+ PortRange.parse(input);
+ return true;
+ } catch (IllegalArgumentException e) {
+ return false;
+ }
+ } else {
+ return false;
+ }
+ }
+
+ public static Pair<Integer,Integer> parse(String portRange) {
+ int idx = portRange.indexOf('-');
+ if (idx != -1) {
+ int low = Integer.parseInt(portRange.substring(0, idx));
+ if (low < 1024) {
+ log.error("Invalid port number for low end of the range, using 1024");
+ low = 1024;
+ }
+ int high = Integer.parseInt(portRange.substring(idx + 1));
+ if (high > 65535) {
--- End diff --
what about case where `low > 65535`
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on the issue:
https://github.com/apache/accumulo/pull/111
I'll take a look, thanks @ShawnWalker
---
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 #111: ACCUMULO-4331: first draft
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/111#discussion_r67010241
--- Diff: core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java ---
@@ -262,4 +267,46 @@ public boolean apply(final String input) {
}
+ public static class PortRange extends Matches {
+
+ private static final Logger log = LoggerFactory.getLogger(PortRange.class);
+
+ public PortRange(final String pattern) {
+ super(pattern);
+ }
+
+ @Override
+ public boolean apply(final String input) {
+ if (super.apply(input)) {
+ try {
+ PortRange.parse(input);
+ return true;
+ } catch (IllegalArgumentException e) {
+ return false;
+ }
+ } else {
+ return false;
+ }
+ }
+
+ public static Pair<Integer,Integer> parse(String portRange) {
+ int idx = portRange.indexOf('-');
+ if (idx != -1) {
+ int low = Integer.parseInt(portRange.substring(0, idx));
+ if (low < 1024) {
+ log.error("Invalid port number for low end of the range, using 1024");
+ low = 1024;
+ }
+ int high = Integer.parseInt(portRange.substring(idx + 1));
--- End diff --
should there be a check that `low<high`?
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r67006667
--- Diff: core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java ---
@@ -72,4 +72,79 @@ public void testGetPropertyByString() {
}
assertTrue("test was a dud, and did nothing", found);
}
+
+ @Test
+ public void testGetSinglePort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(9997, ports[0]);
+ }
+
+ @Test
+ public void testGetAnyPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "0");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(0, ports[0]);
+ }
+
+ @Test
+ public void testGetInvalidPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(Integer.parseInt(Property.TSERV_CLIENTPORT.getDefaultValue()), ports[0]);
+ }
+
+ @Test
+ public void testGetPortRange() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997-9999");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(9997, ports[0]);
+ assertEquals(9998, ports[1]);
+ assertEquals(9999, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidLow() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020-1026");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(1024, ports[0]);
+ assertEquals(1025, ports[1]);
+ assertEquals(1026, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidHigh() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "65533-65538");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(65533, ports[0]);
+ assertEquals(65534, ports[1]);
+ assertEquals(65535, ports[2]);
+ }
+
+ @Test(expected = IllegalArgumentException.class)
--- End diff --
Correct. My last comment on the issue was regarding this.
---
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 #111: ACCUMULO-4331: first draft
Posted by dlmarion <gi...@git.apache.org>.
Github user dlmarion commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/111#discussion_r67004086
--- Diff: core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java ---
@@ -72,4 +72,79 @@ public void testGetPropertyByString() {
}
assertTrue("test was a dud, and did nothing", found);
}
+
+ @Test
+ public void testGetSinglePort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(9997, ports[0]);
+ }
+
+ @Test
+ public void testGetAnyPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "0");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(0, ports[0]);
+ }
+
+ @Test
+ public void testGetInvalidPort() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(1, ports.length);
+ assertEquals(Integer.parseInt(Property.TSERV_CLIENTPORT.getDefaultValue()), ports[0]);
+ }
+
+ @Test
+ public void testGetPortRange() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "9997-9999");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(9997, ports[0]);
+ assertEquals(9998, ports[1]);
+ assertEquals(9999, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidLow() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "1020-1026");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(1024, ports[0]);
+ assertEquals(1025, ports[1]);
+ assertEquals(1026, ports[2]);
+ }
+
+ @Test
+ public void testGetPortRangeInvalidHigh() {
+ AccumuloConfiguration c = AccumuloConfiguration.getDefaultConfiguration();
+ ConfigurationCopy cc = new ConfigurationCopy(c);
+ cc.set(Property.TSERV_CLIENTPORT, "65533-65538");
+ int[] ports = cc.getPort(Property.TSERV_CLIENTPORT);
+ assertEquals(3, ports.length);
+ assertEquals(65533, ports[0]);
+ assertEquals(65534, ports[1]);
+ assertEquals(65535, ports[2]);
+ }
+
+ @Test(expected = IllegalArgumentException.class)
--- End diff --
testGetPortRangeInvalidHigh does not throw an error. The current code preserves what the previous code did, it guards against any port outside of 1024-65535. testGetPortRangeInvalidLow does the same. It truncates the range to a valid set of port numbers.
---
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.
---