[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
GitHub user milleruntime opened a pull request:

    https://github.com/apache/accumulo/pull/267

    ACCUMULO-4642 Changes to remove ServerConfiguration.getInstance()

    A first step in the bigger refactoring effort for [ACCUMULO-4050](https://issues.apache.org/jira/browse/ACCUMULO-4050).  These changes remove ServerConfiguration.getInstance() and make more use of AccumuloServerContext across TableBalancer classes.  Also deprecates TabletBalancer.init(ServerConfigurationFactory conf) replacing with init(AccumuloServerContext context) to assist in the configuration changes.  
   
    Even without the completion of ACCUMULO-4050, these changes will help simplify the configuration internals of Accumulo.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/milleruntime/accumulo ACCUMULO-4642

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/accumulo/pull/267.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 #267
   
----
commit 5de9951537c9a26335eda79ca7303d6e0950997e
Author: Mike Miller <[hidden email]>
Date:   2017-05-31T20:41:24Z

    ACCUMULO-4642 Changes to remove ServerConfiguration.getInstance()

----


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r120977173
 
    --- Diff: server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java ---
    @@ -71,7 +71,7 @@ public void setUp() {
         opts = new Opts();
         systemConfig = createSystemConfig();
         ServerConfigurationFactory factory = createMock(ServerConfigurationFactory.class);
    -    expect(factory.getInstance()).andReturn(instance).anyTimes();
    +    // expect(factory.getInstance()).andReturn(instance).anyTimes();
    --- End diff --
   
    Remove 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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r120977854
 
    --- Diff: server/tracer/src/main/java/org/apache/accumulo/tracer/TraceServer.java ---
    @@ -169,23 +170,25 @@ public void span(RemoteSpan s) throws TException {
             if (timeMutation != null)
               writer.addMutation(timeMutation);
           } catch (MutationsRejectedException exception) {
    -        log.warn("Unable to write mutation to table; discarding span. set log level to DEBUG for span information and stacktrace. cause: " + exception);
    +        log.warn("Unable to write mutation to table; discarding span. set log level to DEBUG for span information and stacktrace. cause: {}", exception,
    --- End diff --
   
    While these log message changes are fine, can you please separate them into another JIRA issue?


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r120977502
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/master/balancer/TabletBalancer.java ---
    @@ -55,16 +56,25 @@
     
       private static final Logger log = LoggerFactory.getLogger(TabletBalancer.class);
     
    -  protected ServerConfigurationFactory configuration;
    -
       protected AccumuloServerContext context;
     
       /**
        * Initialize the TabletBalancer. This gives the balancer the opportunity to read the configuration.
    +   *
    +   * @deprecated since 2.0.0; use {@link #init(AccumuloServerContext)} instead.
    --- End diff --
   
    Nice javadoc


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r120977606
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/master/balancer/ChaoticLoadBalancer.java ---
    @@ -162,7 +163,16 @@ public long balance(SortedMap<TServerInstance,TabletServerStatus> current, Set<K
         return 100;
       }
     
    +  /**
    +   * Initialize the TabletBalancer. This gives the balancer the opportunity to read the configuration.
    --- End diff --
   
    Don't need to duplicate the javadoc here. Most IDEs will inherit it from TabletBalancer


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r120996306
 
    --- Diff: server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java ---
    @@ -71,7 +71,7 @@ public void setUp() {
         opts = new Opts();
         systemConfig = createSystemConfig();
         ServerConfigurationFactory factory = createMock(ServerConfigurationFactory.class);
    -    expect(factory.getInstance()).andReturn(instance).anyTimes();
    +    // expect(factory.getInstance()).andReturn(instance).anyTimes();
    --- End diff --
   
    Good eye, thanks.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r120996472
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/master/balancer/ChaoticLoadBalancer.java ---
    @@ -162,7 +163,16 @@ public long balance(SortedMap<TServerInstance,TabletServerStatus> current, Set<K
         return 100;
       }
     
    +  /**
    +   * Initialize the TabletBalancer. This gives the balancer the opportunity to read the configuration.
    --- End diff --
   
    Agreed.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r120997737
 
    --- Diff: server/tracer/src/main/java/org/apache/accumulo/tracer/TraceServer.java ---
    @@ -169,23 +170,25 @@ public void span(RemoteSpan s) throws TException {
             if (timeMutation != null)
               writer.addMutation(timeMutation);
           } catch (MutationsRejectedException exception) {
    -        log.warn("Unable to write mutation to table; discarding span. set log level to DEBUG for span information and stacktrace. cause: " + exception);
    +        log.warn("Unable to write mutation to table; discarding span. set log level to DEBUG for span information and stacktrace. cause: {}", exception,
    --- End diff --
   
    I pulled these changes from a patch supplied by Christopher and I think some of these got snagged with them.  I guess I can create a ticket with something like "utilize slf4j"?  A quick search in JIRA didn't reveal any tickets like 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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user joshelser commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r120998514
 
    --- Diff: server/tracer/src/main/java/org/apache/accumulo/tracer/TraceServer.java ---
    @@ -169,23 +170,25 @@ public void span(RemoteSpan s) throws TException {
             if (timeMutation != null)
               writer.addMutation(timeMutation);
           } catch (MutationsRejectedException exception) {
    -        log.warn("Unable to write mutation to table; discarding span. set log level to DEBUG for span information and stacktrace. cause: " + exception);
    +        log.warn("Unable to write mutation to table; discarding span. set log level to DEBUG for span information and stacktrace. cause: {}", exception,
    --- End diff --
   
    Yeah, I'd appreciate just creating one for yourself. It's much easier to cherry-pick the changes later and easier on the eyes now.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r121002215
 
    --- Diff: server/tracer/src/main/java/org/apache/accumulo/tracer/TraceServer.java ---
    @@ -169,23 +170,25 @@ public void span(RemoteSpan s) throws TException {
             if (timeMutation != null)
               writer.addMutation(timeMutation);
           } catch (MutationsRejectedException exception) {
    -        log.warn("Unable to write mutation to table; discarding span. set log level to DEBUG for span information and stacktrace. cause: " + exception);
    +        log.warn("Unable to write mutation to table; discarding span. set log level to DEBUG for span information and stacktrace. cause: {}", exception,
    --- End diff --
   
    No problem, will do.  I now remember grep'ing the code for occurrences of log string concatenation that could be replaced by slf4j logging but quickly gave up, haha.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #267: ACCUMULO-4642 Changes to remove ServerConfiguration.get...

joshelser
In reply to this post by joshelser
Github user joshelser commented on the issue:

    https://github.com/apache/accumulo/pull/267
 
    Cool beans. Thanks for the quick turn-around, Mike. Looks fine to me otherwise.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r121007535
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/conf/TableConfWatcher.java ---
    @@ -33,11 +33,13 @@
       private static final Logger log = Logger.getLogger(TableConfWatcher.class);
       private final Instance instance;
       private final String tablesPrefix;
    +  private final int tablesPrefixLength;
       private ServerConfigurationFactory scf;
     
       TableConfWatcher(Instance instance) {
         this.instance = instance;
         tablesPrefix = ZooUtil.getRoot(instance) + Constants.ZTABLES + "/";
    +    tablesPrefixLength = tablesPrefix.length();
    --- End diff --
   
    Why store the length?


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r121026193
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/conf/TableConfWatcher.java ---
    @@ -33,11 +33,13 @@
       private static final Logger log = Logger.getLogger(TableConfWatcher.class);
       private final Instance instance;
       private final String tablesPrefix;
    +  private final int tablesPrefixLength;
       private ServerConfigurationFactory scf;
     
       TableConfWatcher(Instance instance) {
         this.instance = instance;
         tablesPrefix = ZooUtil.getRoot(instance) + Constants.ZTABLES + "/";
    +    tablesPrefixLength = tablesPrefix.length();
    --- End diff --
   
    I think this got carried in from my initial patch that I had attached to the JIRA that @milleruntime picked up. I think it was part of an unrelated change that got picked up in the diff. I actually discarded that change from my workspace because I couldn't remember why I had originally done this. :smile:


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r121026293
 
    --- Diff: server/tracer/src/main/java/org/apache/accumulo/tracer/TraceServer.java ---
    @@ -169,23 +170,25 @@ public void span(RemoteSpan s) throws TException {
             if (timeMutation != null)
               writer.addMutation(timeMutation);
           } catch (MutationsRejectedException exception) {
    -        log.warn("Unable to write mutation to table; discarding span. set log level to DEBUG for span information and stacktrace. cause: " + exception);
    +        log.warn("Unable to write mutation to table; discarding span. set log level to DEBUG for span information and stacktrace. cause: {}", exception,
    --- End diff --
   
    Yeah, sorry about that. The patch I attached to the JIRA included some other changes I had made in my workspace which were unrelated.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r121167952
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/conf/TableConfWatcher.java ---
    @@ -33,11 +33,13 @@
       private static final Logger log = Logger.getLogger(TableConfWatcher.class);
       private final Instance instance;
       private final String tablesPrefix;
    +  private final int tablesPrefixLength;
       private ServerConfigurationFactory scf;
     
       TableConfWatcher(Instance instance) {
         this.instance = instance;
         tablesPrefix = ZooUtil.getRoot(instance) + Constants.ZTABLES + "/";
    +    tablesPrefixLength = tablesPrefix.length();
    --- End diff --
   
    Fixed with 1598590


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user milleruntime commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/267#discussion_r121169020
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/master/balancer/ChaoticLoadBalancer.java ---
    @@ -162,7 +163,16 @@ public long balance(SortedMap<TServerInstance,TabletServerStatus> current, Set<K
         return 100;
       }
     
    +  /**
    +   * Initialize the TabletBalancer. This gives the balancer the opportunity to read the configuration.
    --- End diff --
   
    Fixed with d879158


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo issue #267: ACCUMULO-4642 Changes to remove ServerConfiguration.get...

joshelser
In reply to this post by joshelser
Github user milleruntime commented on the issue:

    https://github.com/apache/accumulo/pull/267
 
    These changes ran successfully yesterday with Sunny ITs on my laptop.  Since it looks like Jenkins is still borked, I will just go ahead with pushing these changes today.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] accumulo pull request #267: ACCUMULO-4642 Changes to remove ServerConfigurat...

joshelser
In reply to this post by joshelser
Github user asfgit closed the pull request at:

    https://github.com/apache/accumulo/pull/267


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---
Loading...