You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/04/07 16:19:14 UTC
[GitHub] [accumulo] ivakegg opened a new pull request #2006: Bugfix/hostregexconfig
ivakegg opened a new pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006
Ensure that the HostRegexTableLoadBalancer property changes are picked up.
When properties are set at the system level for the HostRegexTableLoadBalancer, the changes are not picked up and require the master to be restarted. This change ensures that the property changes are always picked up whenever balance or getAssignments is called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#issuecomment-824220356
I merged this forward into the main branch, as is our practice to ensure we don't have regressions in newer versions for failing to include all fixes from previous branches. Please remember to do this in future if you commit to an older branch. In this case, it was a no-op merge, because this change only applied to 1.10.
Also, please ensure you have a blank line between the "subject" and the "body" of your git commits, so the commit message doesn't get mangled in the `git log --oneline` view and in other utilities that show git history. See https://chris.beams.io/posts/git-commit/ for some good advice on formatting git commit messages, or [my email to the dev list in February](https://lists.apache.org/thread.html/r5773a166ef2ada07a9ad769af0e1b729614312e763aac80961d5051a%40%3Cdev.accumulo.apache.org%3E) with additional tips.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#discussion_r608824695
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -252,6 +252,14 @@ protected void parseConfiguration(ServerConfiguration conf) {
tableIdToTableName = ImmutableMap.copyOf(tableIdToTableNameBuilder);
poolNameToRegexPattern = ImmutableMap.copyOf(poolNameToRegexPatternBuilder);
+ LOG.info("{}", this);
Review comment:
Is this necessary, or was it left in during development?
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -220,7 +220,7 @@ protected String getPoolNameForTable(String tableName) {
* @param conf
* server configuration
*/
- protected void parseConfiguration(ServerConfiguration conf) {
Review comment:
Probably not a good idea to rename the protected method. Although this isn't public API, we have attempted to keep balancer APIs somewhat stable to reduce disruption.
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -252,6 +252,14 @@ protected void parseConfiguration(ServerConfiguration conf) {
tableIdToTableName = ImmutableMap.copyOf(tableIdToTableNameBuilder);
poolNameToRegexPattern = ImmutableMap.copyOf(poolNameToRegexPatternBuilder);
+ LOG.info("{}", this);
+ }
+
+ public void parseSystemConfiguration(ServerConfiguration conf) {
Review comment:
Should this new method be public?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#discussion_r611208413
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -220,7 +220,7 @@ protected String getPoolNameForTable(String tableName) {
* @param conf
* server configuration
*/
- protected void parseConfiguration(ServerConfiguration conf) {
Review comment:
Yes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#discussion_r610797799
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -252,6 +252,14 @@ protected void parseConfiguration(ServerConfiguration conf) {
tableIdToTableName = ImmutableMap.copyOf(tableIdToTableNameBuilder);
poolNameToRegexPattern = ImmutableMap.copyOf(poolNameToRegexPatternBuilder);
+ LOG.info("{}", this);
+ }
+
+ public void parseSystemConfiguration(ServerConfiguration conf) {
Review comment:
no, I will change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ivakegg commented on pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#issuecomment-816840304
> Also, have you looked into how these would apply to the latest branch? I think these classes may have changed in the newer branch, and it may help determine the appropriate 1.10 implementation to contextualize the target implementation in the main branch.
This change can be simply dropped when merging into 2.0 because the whole configuration mechanism changed there and the AccumuloConfiguration.Deriver mechanism takes care of this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#discussion_r610819566
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -220,7 +220,7 @@ protected String getPoolNameForTable(String tableName) {
* @param conf
* server configuration
*/
- protected void parseConfiguration(ServerConfiguration conf) {
Review comment:
Should we even bother with the observer rebuild, though, if we're calling `parseSystemConfiguration` every time we balance, which calls `parseTableConfiguration` every time? The point of the observer was to avoid reparsing the config unless there was a change. Now, the config is reparsed every time, no matter what, *plus* when the config changes.
Also, note that the observer interface stuff changed in the main branch. And, I believe it may have changed in a way that would work for detecting changes to the system config as well, but I'd have to check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#discussion_r610798225
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -252,6 +252,14 @@ protected void parseConfiguration(ServerConfiguration conf) {
tableIdToTableName = ImmutableMap.copyOf(tableIdToTableNameBuilder);
poolNameToRegexPattern = ImmutableMap.copyOf(poolNameToRegexPatternBuilder);
+ LOG.info("{}", this);
Review comment:
It put it there because the other configuration mechanism had the same thing. I thing it is reasonable to have a record of when the new configuration is picked up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ivakegg merged pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ivakegg merged pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] elsaxo commented on pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
elsaxo commented on pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#issuecomment-817111168
> Ensure that the HostRegexTableLoadBalancer property changes are picked up.
> When properties are set at the system level for the HostRegexTableLoadBalancer, the changes are not picked up and require the master to be restarted. This change ensures that the property changes are always picked up whenever balance or getAssignments is called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#discussion_r613366737
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -85,7 +85,7 @@
* <b>table.custom.balancer.host.regex.max.outstanding.migrations</b>
*
*/
-public class HostRegexTableLoadBalancer extends TableLoadBalancer implements ConfigurationObserver {
Review comment:
Removing this interface could cause user code that assigns the variable to a type, or any subclasses that explicitly override those methods to fail to compile. I think it's probably low-risk enough that it's okay, but just wanted to raise the point for consideration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#discussion_r610796771
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -220,7 +220,7 @@ protected String getPoolNameForTable(String tableName) {
* @param conf
* server configuration
*/
- protected void parseConfiguration(ServerConfiguration conf) {
Review comment:
> If I'm understanding these changes correctly, it seems the method naming/refactoring isn't the important bit. Was that done for testing purposes or some other reason?
>
> The most minimal versions of these changes seem to be to simply add two additional `parseConfiguration` calls to the `getAssignments` and `balance` methods. Can we go with the more minimal change without the method renaming/new method?
The reason for the separate methods is because the Observer interface is only for changes to the table configuration and hence updating system properties from those listener methods is not needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#discussion_r610870166
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -220,7 +220,7 @@ protected String getPoolNameForTable(String tableName) {
* @param conf
* server configuration
*/
- protected void parseConfiguration(ServerConfiguration conf) {
Review comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#discussion_r610915355
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -220,7 +220,7 @@ protected String getPoolNameForTable(String tableName) {
* @param conf
* server configuration
*/
- protected void parseConfiguration(ServerConfiguration conf) {
Review comment:
Are we okay with the performance hit, if any?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a change in pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ivakegg commented on a change in pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#discussion_r610860719
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -220,7 +220,7 @@ protected String getPoolNameForTable(String tableName) {
* @param conf
* server configuration
*/
- protected void parseConfiguration(ServerConfiguration conf) {
Review comment:
That is a very fair observation. Thank you. I will remove the observer mechanism.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2006: Ensure that the HostRegexTableLoadBalancer property changes are picked up
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2006:
URL: https://github.com/apache/accumulo/pull/2006#discussion_r610819566
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/HostRegexTableLoadBalancer.java
##########
@@ -220,7 +220,7 @@ protected String getPoolNameForTable(String tableName) {
* @param conf
* server configuration
*/
- protected void parseConfiguration(ServerConfiguration conf) {
Review comment:
Should we even bother with the observer rebuild, though, if we're calling `parseSystemConfiguration` every time we balance, which calls `parseTableConfiguration` every time? The point of the observer was to avoid reparsing the config unless there was a change. Now, the config is reparsed every time, no matter what, *plus* when the config changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org