[GitHub] [accumulo-testing] phrocker edited a comment on pull request #118: Adding Accumulo Availability Monitor
phrocker edited a comment on pull request #118: URL: https://github.com/apache/accumulo-testing/pull/118#issuecomment-620939217 > > > @phrocker You can also just do a squash and merge from the GitHub UI... that also gives you a chance to edit the commit message. This one wasn't well-formatted. ah yes. I wanted to sign the commit but after pushing ( recently chided on another project for forgetting to do this )-- but I realized that wasn't necessary here after pushing during key verification of this and other commits-- so yeah ... I should have used the UI ( though in reading about it I think that may be possible ). Either way, thanks! 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: us...@infra.apache.org
[GitHub] [accumulo-testing] phrocker commented on pull request #118: Adding Accumulo Availability Monitor
phrocker commented on pull request #118: URL: https://github.com/apache/accumulo-testing/pull/118#issuecomment-620939217 > > > @phrocker You can also just do a squash and merge from the GitHub UI... that also gives you a chance to edit the commit message. This one wasn't well-formatted. ah yes. I wanted to sign the commit but after pushing I realized that wasn't necessary ( by way of seeing that other commits weren't signed )-- so yeah ... I should have used the UI ( though in reading about it I think that may be possible ). Either way, thanks! 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: us...@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1599: Fix #1598 - Reduce MasterMetricsIT test resources.
ctubbsii commented on a change in pull request #1599: URL: https://github.com/apache/accumulo/pull/1599#discussion_r417007121 ## File path: test/src/main/java/org/apache/accumulo/test/functional/MasterMetricsIT.java ## @@ -60,7 +60,7 @@ private static final long TAIL_DELAY = 5_000; // number of tables / concurrent compactions used during testing. - private final int tableCount = 4; + private final int tableCount = 1; Review comment: Never mind, I read the comments on #1598 after I looked at this code. 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: us...@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1599: Fix #1598 - Reduce MasterMetricsIT test resources.
ctubbsii commented on a change in pull request #1599: URL: https://github.com/apache/accumulo/pull/1599#discussion_r417006665 ## File path: test/src/main/java/org/apache/accumulo/test/functional/MasterMetricsIT.java ## @@ -60,7 +60,7 @@ private static final long TAIL_DELAY = 5_000; // number of tables / concurrent compactions used during testing. - private final int tableCount = 4; + private final int tableCount = 1; Review comment: Does this affect the behavior of the test at all? 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: us...@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1602: Lambdas replaced with method references.
ctubbsii commented on a change in pull request #1602:
URL: https://github.com/apache/accumulo/pull/1602#discussion_r417005599
##
File path:
server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java
##
@@ -353,7 +353,7 @@ public Path matchingFileSystem(Path source, Set
options) {
URI optUri = URI.create(opt);
return sourceUri.getScheme().equals(optUri.getScheme())
&& Objects.equals(sourceUri.getAuthority(), optUri.getAuthority());
-}).map((String opt) -> new Path(opt)).findFirst().orElse(null);
+}).map(Path::new).findFirst().orElse(null);
Review comment:
Path has strange and subtle differences between different constructors.
The original code specified the constructor was the one that took a String
explicitly. This code would work if the type was changed to URI, instead of
String, but I'm not sure the behavior would be identical. The original version
protects against subtle differences like that. For this reason, I'm not a big
fan of method references for constructors, unless the type is *very* stable and
well-known (like `ArrayList::new`).
The change you've made here is probably fine... but I'm not sure if we want
to protect against possible subtle changes in this code by keeping the explicit
String type declaration.
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:
us...@infra.apache.org
[GitHub] [accumulo-testing] ctubbsii commented on pull request #118: Adding Accumulo Availability Monitor
ctubbsii commented on pull request #118: URL: https://github.com/apache/accumulo-testing/pull/118#issuecomment-620912941 @phrocker You can also just do a squash and merge from the GitHub UI... that also gives you a chance to edit the commit message. This one wasn't well-formatted. 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: us...@infra.apache.org
[GitHub] [accumulo-website] ctubbsii commented on pull request #229: Add 'Tushar Dhadiwal' to people.md
ctubbsii commented on pull request #229: URL: https://github.com/apache/accumulo-website/pull/229#issuecomment-620826271 Re: apache/accumulo#1594 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: us...@infra.apache.org
[GitHub] [accumulo] jmark99 opened a new pull request #1602: Lambdas replaced with method references.
jmark99 opened a new pull request #1602: URL: https://github.com/apache/accumulo/pull/1602 This ticket updates instances of lambdas which can be replaced with method references. There was also one instance of an anonymous type being replaced with a method reference. 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: us...@infra.apache.org
[GitHub] [accumulo-proxy] volmasoft opened a new issue #21: Provide overrides for proxy.properties in Docker deployments
volmasoft opened a new issue #21: URL: https://github.com/apache/accumulo-proxy/issues/21 @keith-turner and I briefly touched upon allowing `proxy.properties` to be overidden for a Docker instantiation of accumulo-proxy in this pull request: https://github.com/apache/accumulo-proxy/pull/20 Currently in accumulo-docker you can override properties by the following: ``` export ACCUMULO_CL_OPTS="-o prop.1=var1 -o prop.2=var2" docker run -d --network="host" accumulo monitor $ACCUMULO_CL_OPTS ``` This is documented in the docs here: https://github.com/apache/accumulo-docker For standard docker approaches I would say this is a simplistic mechanism that works really cleanly however if you start to look at things like docker-compose or kubernetes I would say it would be advantageous to avoid overriding the CMD or ENTRYPOINT flags where possible. An alternative method would be to provide these as environment variables, therefore you could also use the Kubernetes secret APIs to handle things such as passwords and either map them to a file or environment variable for use within the application. Would this be acceptable? If so I'm also happy to look at providing the same change to accumulo-docker to retain consistency? 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: us...@infra.apache.org
[GitHub] [accumulo-proxy] volmasoft commented on a change in pull request #20: WIP: accumulo-proxy docker container
volmasoft commented on a change in pull request #20: URL: https://github.com/apache/accumulo-proxy/pull/20#discussion_r416804624 ## File path: DOCKER.md ## @@ -0,0 +1,53 @@ + + +# accumulo-proxy-docker + +A temporary guide on how to run this up in Docker. + +## Build the image using +Invoke the docker build command to create a container image. +```commandline +docker build -t accumulo-proxy:latest . +``` + +## Default Configuration +By default, the container image expects the following to be true: +1. Your accumulo instance is named "myinstance" +2. Your zookeeper is available (and reachable from the container) at localhost:2181 + +## Custom proxy.properties +If you wish to create advanced proxy.properties configuration changes, you should look to volume mount these in when you invoke the `docker run` command, an example is: +```commandline +docker run --rm -d -p 42424:42424 -v /path/to/proxy.properties:/opt/accumulo-proxy/conf/proxy.properties --add-host"FQDN:IP" --name accumulo-proxy accumulo-proxy:latest +``` + +## Accumulo Instance Configuration +In order for Thrift to communicate with the Accumulo instance you will need to provide the container with the FQDN and IP of the Accumulo instance and its relevant servers (e.g. tservers) to allow it to resolve the required DNS. + +If you are running an Accumulo instance with more than one tserver you should add each tserver's entry with a new `--add-host "FQDN:IP"` entry. Review comment: I'm going to raise an issue to capture the proxy.properties overrides from the command line, I would like to discuss potential options before implementing these. 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: us...@infra.apache.org
[GitHub] [accumulo-proxy] volmasoft commented on a change in pull request #20: WIP: accumulo-proxy docker container
volmasoft commented on a change in pull request #20: URL: https://github.com/apache/accumulo-proxy/pull/20#discussion_r416803798 ## File path: DOCKER.md ## @@ -0,0 +1,53 @@ + + +# accumulo-proxy-docker + +A temporary guide on how to run this up in Docker. + +## Build the image using +Invoke the docker build command to create a container image. +```commandline +docker build -t accumulo-proxy:latest . +``` + +## Default Configuration +By default, the container image expects the following to be true: +1. Your accumulo instance is named "myinstance" +2. Your zookeeper is available (and reachable from the container) at localhost:2181 + +## Custom proxy.properties +If you wish to create advanced proxy.properties configuration changes, you should look to volume mount these in when you invoke the `docker run` command, an example is: +```commandline +docker run --rm -d -p 42424:42424 -v /path/to/proxy.properties:/opt/accumulo-proxy/conf/proxy.properties --add-host"FQDN:IP" --name accumulo-proxy accumulo-proxy:latest +``` + +## Accumulo Instance Configuration +In order for Thrift to communicate with the Accumulo instance you will need to provide the container with the FQDN and IP of the Accumulo instance and its relevant servers (e.g. tservers) to allow it to resolve the required DNS. + +If you are running an Accumulo instance with more than one tserver you should add each tserver's entry with a new `--add-host "FQDN:IP"` entry. Review comment: Take a look at the latest update, I decided to re-write the whole section covering networking to try and give a bit more clarity around options but I also tried to avoid re-writing the docker documentation, hopefully this strikes a balance. 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: us...@infra.apache.org
[GitHub] [accumulo-website] tushardhadiwal opened a new pull request #229: Add 'Tushar Dhadiwal' to people.md
tushardhadiwal opened a new pull request #229: URL: https://github.com/apache/accumulo-website/pull/229 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: us...@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #1594: Handle spurious failure were rename succeeded but reported it failed (#1588)
keith-turner commented on pull request #1594: URL: https://github.com/apache/accumulo/pull/1594#issuecomment-620698845 @tushardhadiwal thanks for the contribution. If you would like to be listed as a contributor on the [people page](https://accumulo.apache.org/people), please [submit a PR to add yourself](https://github.com/apache/accumulo-website/edit/master/pages/people.md). Let me know if you have any questions about editing the website. 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: us...@infra.apache.org
Accumulo-Master - Build # 3102 - Fixed
The Apache Jenkins build system has built Accumulo-Master (build #3102) Status: Fixed Check console output at https://builds.apache.org/job/Accumulo-Master/3102/ to view the results.
[GitHub] [accumulo-proxy] keith-turner commented on a change in pull request #20: WIP: accumulo-proxy docker container
keith-turner commented on a change in pull request #20: URL: https://github.com/apache/accumulo-proxy/pull/20#discussion_r416662601 ## File path: DOCKER.md ## @@ -0,0 +1,53 @@ + + +# accumulo-proxy-docker + +A temporary guide on how to run this up in Docker. + +## Build the image using +Invoke the docker build command to create a container image. +```commandline +docker build -t accumulo-proxy:latest . +``` + +## Default Configuration +By default, the container image expects the following to be true: +1. Your accumulo instance is named "myinstance" +2. Your zookeeper is available (and reachable from the container) at localhost:2181 + +## Custom proxy.properties +If you wish to create advanced proxy.properties configuration changes, you should look to volume mount these in when you invoke the `docker run` command, an example is: +```commandline +docker run --rm -d -p 42424:42424 -v /path/to/proxy.properties:/opt/accumulo-proxy/conf/proxy.properties --add-host"FQDN:IP" --name accumulo-proxy accumulo-proxy:latest +``` + +## Accumulo Instance Configuration +In order for Thrift to communicate with the Accumulo instance you will need to provide the container with the FQDN and IP of the Accumulo instance and its relevant servers (e.g. tservers) to allow it to resolve the required DNS. + +If you are running an Accumulo instance with more than one tserver you should add each tserver's entry with a new `--add-host "FQDN:IP"` entry. Review comment: Could have a sentence like the following. ``` Instead of having an `--add-host` option for each Accumulo server, a single `--network="host"` may work in some environments. ``` 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: us...@infra.apache.org
[GitHub] [accumulo-proxy] volmasoft commented on a change in pull request #20: WIP: accumulo-proxy docker container
volmasoft commented on a change in pull request #20: URL: https://github.com/apache/accumulo-proxy/pull/20#discussion_r416597585 ## File path: DOCKER.md ## @@ -0,0 +1,53 @@ + + +# accumulo-proxy-docker + +A temporary guide on how to run this up in Docker. + +## Build the image using +Invoke the docker build command to create a container image. +```commandline +docker build -t accumulo-proxy:latest . +``` + +## Default Configuration +By default, the container image expects the following to be true: +1. Your accumulo instance is named "myinstance" +2. Your zookeeper is available (and reachable from the container) at localhost:2181 + +## Custom proxy.properties +If you wish to create advanced proxy.properties configuration changes, you should look to volume mount these in when you invoke the `docker run` command, an example is: +```commandline +docker run --rm -d -p 42424:42424 -v /path/to/proxy.properties:/opt/accumulo-proxy/conf/proxy.properties --add-host"FQDN:IP" --name accumulo-proxy accumulo-proxy:latest +``` + +## Accumulo Instance Configuration +In order for Thrift to communicate with the Accumulo instance you will need to provide the container with the FQDN and IP of the Accumulo instance and its relevant servers (e.g. tservers) to allow it to resolve the required DNS. + +If you are running an Accumulo instance with more than one tserver you should add each tserver's entry with a new `--add-host "FQDN:IP"` entry. Review comment: @keith-turner so that's true in certain environments, this is where I think we have to make a decision about what to support/not support. In standard linux generally the docker configuration is fairly close to bare metal. If you use Windows or in my case Mac generally you run Docker 'technically' inside a VM, therefore when you attach the host network it's still not technically the 'host (physical machine)' network. So whilst adding `--network="host"` into the instructions it'll only actually work for a subset of environments, adding the `--add-host` would work for a wider number and also allows for some isolation of resources (maybe a step too far for dev). Also this all changes again if we look towards any other orchestration tooling such as docker-compose or Kubernetes as both of those have different working configurations which then need different tuning to work. --- Given this is starting to creep into environment specific, I might comment on both options in the DOCKER.md and let the caller decide how they want to start their environment, but I'll try and give some hints as to what people may wish to consider. For now, I don't think many of us are intending Docker to be a simple non-technically orientated mechanism to setup so we can probably get away with giving options for now. 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: us...@infra.apache.org
[GitHub] [accumulo] jmark99 opened a new pull request #1601: Replace Collections.sort() with List.sort()
jmark99 opened a new pull request #1601: URL: https://github.com/apache/accumulo/pull/1601 As of JDK 1.8, calls to Collections.sort(list, comparator) can be replaced with List.sort(comparator). 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: us...@infra.apache.org
