[GitHub] accumulo pull request #204: ACCUMULO-4574 Modified TableOperations online to...

[EdColeman]

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

https://github.com/apache/accumulo/pull/204#discussion_r98584001
  
--- Diff: 
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java
 ---
@@ -1208,11 +1207,20 @@ public void online(String tableName) throws 
AccumuloSecurityException, AccumuloE
   @Override
   public void online(String tableName, boolean wait) throws 
AccumuloSecurityException, AccumuloException, TableNotFoundException {
 checkArgument(tableName != null, "tableName is null");
+
+long cacheResetCount = Tables.getCacheResetCount();
+
 String tableId = Tables.getTableId(context.getInstance(), tableName);
 
 /**
  * ACCUMULO-4574 if table is already online return without executing 
fate operation.
  */
+
+// getTableId may have side effect of clearing zookeeper cache - no 
need to clear again.
+if (cacheResetCount == Tables.getCacheResetCount()) {
+  Tables.clearCacheByPath(context.getInstance(), Constants.ZTABLES + 
"/" + tableId + Constants.ZTABLE_STATE);
--- End diff --

I have no issue with incorporating this suggestion - leaning towards a 
getTableState state method with a boolean - however,  a question: Are there 
other places that would benefit from having the general method? I was assuming 
that with by passing in a generic path the method would be more general and 
could be used elsewhere.  Or, are you suggesting that we keep both? I have no 
strong opinion either way.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #204: ACCUMULO-4574 Modified TableOperations online to...

[EdColeman]

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

https://github.com/apache/accumulo/pull/204#discussion_r98583191
  
--- Diff: 
core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java ---
@@ -149,6 +149,28 @@ public static void clearCache(Instance instance) {
 getZooCache(instance).clear(ZooUtil.getRoot(instance) + 
Constants.ZNAMESPACES);
   }
 
+  /**
+   * Clears the zoo cache from instance/root/{PATH}
+   *
+   * @param instance
+   *  The Accumulo Instance
+   * @param zooPath
+   *  A zookeeper path
+   */
+  public static void clearCacheByPath(Instance instance, final String 
zooPath) {
+
+String thePath;
+
+if (zooPath.startsWith("/")) {
+  thePath = zooPath;
+} else {
+  thePath = "/" + zooPath;
+}
+
+getZooCache(instance).clear(ZooUtil.getRoot(instance) + thePath);
--- End diff --

I was not sure - so I was only adding the slash if it was not present.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo-testing pull request #3: ACCUMULO-4579 Fixed hadoop config bug in a...

[mikewalch]

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

https://github.com/apache/accumulo-testing/pull/3#discussion_r98556591
  
--- Diff: core/src/main/java/org/apache/accumulo/testing/core/TestEnv.java 
---
@@ -96,15 +97,22 @@ public String getPid() {
   }
 
   public Configuration getHadoopConfiguration() {
-Configuration config = new Configuration();
-config.set("mapreduce.framework.name", "yarn");
-// Setting below are required due to bundled jar breaking default
-// config.
-// See
-// 
http://stackoverflow.com/questions/17265002/hadoop-no-filesystem-for-scheme-file
-config.set("fs.hdfs.impl", 
org.apache.hadoop.hdfs.DistributedFileSystem.class.getName());
-config.set("fs.file.impl", 
org.apache.hadoop.fs.LocalFileSystem.class.getName());
-return config;
+if (hadoopConfig == null) {
+  String hadoopPrefix = System.getenv("HADOOP_PREFIX");
+  if (hadoopPrefix == null || hadoopPrefix.isEmpty()) {
+throw new IllegalArgumentException("HADOOP_PREFIX must be sent in 
env");
+  }
+  hadoopConfig = new Configuration();
+  hadoopConfig.addResource(new Path(hadoopPrefix + 
"/etc/hadoop/core-site.xml"));
--- End diff --

After talking to @keith-turner offline, I am going to submit an update that 
pull the needed Hadoop configuration from the accumulo-testing.properties file.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo-testing pull request #3: ACCUMULO-4579 Fixed hadoop config bug in a...

[mikewalch]

GitHub user mikewalch opened a pull request:

https://github.com/apache/accumulo-testing/pull/3

ACCUMULO-4579 Fixed hadoop config bug in accumulo-testing

* TestEnv now returns Hadoop configuration that is loaded from config files
  but expects HADOOP_PREFIX to be set in environment
* Fixed bug where Twill was being improperly configured to look in wrong
  location for shaded jar when running test application in YARN.

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

$ git pull https://github.com/mikewalch/accumulo-testing accumulo-4579

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

https://github.com/apache/accumulo-testing/pull/3.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 #3


commit dd8cad26c82b7dc4f446873b855fe52d5d87046a
Author: Mike Walch 
Date:   2017-01-30T20:03:21Z

ACCUMULO-4579 Fixed hadoop config bug in accumulo-testing

* TestEnv now returns Hadoop configuration that is loaded from config files
  but expects HADOOP_PREFIX to be set in environment
* Fixed bug where Twill was being improperly configured to look in wrong
  location for shaded jar when running test application in YARN.




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #209: ACCUMULO-4578 release namespace lock when compac...

[asfgit]

Github user asfgit closed the pull request at:

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


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #207: ACCUMULO-4446 Making changes to 1.7

[ctubbsii]

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

https://github.com/apache/accumulo/pull/207#discussion_r98518427
  
--- Diff: 
server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java ---
@@ -642,7 +643,7 @@ private void getMonitorLock() throws KeeperException, 
InterruptedException {
   
UtilWaitThread.sleep(getContext().getConfiguration().getTimeInMillis(Property.MONITOR_LOCK_CHECK_INTERVAL));
 }
 
-log.info("Got Monitor lock.");
+log.info("Acquired Monitor Lock " + monitorLock.getLockPath());
--- End diff --

For what it's worth, Log4J 2.4 and higher support lambdas using a 
`Supplier` interface for this purpose. Java 8 java.util.logging and an SLF4J 
add-on also support this.

I personally find the deferred execution more clean with lambdas than with 
SLF4J's substitution syntax, but switching away from SLF4J in favor of 
something which supports suppliers would have to be a separate issue. If we 
didn't want to switch, we *could* create a simple wrapper facade to support 
deferred execution (also a separate issue, if we wanted to pursue 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #209: ACCUMULO-4578 release namespace lock when compac...

[keith-turner]

Github user keith-turner commented on a diff in the pull request:

https://github.com/apache/accumulo/pull/209#discussion_r98503081
  
--- Diff: fate/src/main/java/org/apache/accumulo/fate/AdminUtil.java ---
@@ -110,25 +110,36 @@ public String getTop() {
   public static class FateStatus {
 
 private final List transactions;
-private final Map danglingHeldLocks;
-private final Map danglingWaitingLocks;
+private final Map danglingHeldLocks;
+private final Map danglingWaitingLocks;
+
+private static Map convert(Map 
danglocks) {
+  if (danglocks.isEmpty()) {
+return Collections.emptyMap();
+  }
+
+  Map ret = new HashMap<>();
+  for (Entry entry : danglocks.entrySet()) {
+ret.put(String.format("%016x", entry.getKey()), 
Collections.unmodifiableList(entry.getValue()));
--- End diff --

> It's not clear, for example, that this is just pre-formatting the object 
for future logging.

Oh, this code is not used by logging.   This code is only used by test and 
the shell.  It uses the same formatting as the other code that does logging.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #209: ACCUMULO-4578 release namespace lock when compac...

[keith-turner]

Github user keith-turner commented on a diff in the pull request:

https://github.com/apache/accumulo/pull/209#discussion_r98502468
  
--- Diff: fate/src/main/java/org/apache/accumulo/fate/AdminUtil.java ---
@@ -110,25 +110,36 @@ public String getTop() {
   public static class FateStatus {
 
 private final List transactions;
-private final Map danglingHeldLocks;
-private final Map danglingWaitingLocks;
+private final Map danglingHeldLocks;
+private final Map danglingWaitingLocks;
+
+private static Map convert(Map 
danglocks) {
+  if (danglocks.isEmpty()) {
+return Collections.emptyMap();
+  }
+
+  Map ret = new HashMap<>();
+  for (Entry entry : danglocks.entrySet()) {
+ret.put(String.format("%016x", entry.getKey()), 
Collections.unmodifiableList(entry.getValue()));
--- End diff --

I added javadocs.  The specific reason I made the change was so that when I 
saw an assertion failure message in a test I could easily find the transaction 
id in the logs.  Before this change I had to manually convert base 10 to base 
16 to find the corresponding info in the logs. 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #209: ACCUMULO-4578 release namespace lock when compac...

[ctubbsii]

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

https://github.com/apache/accumulo/pull/209#discussion_r98489892
  
--- Diff: fate/src/main/java/org/apache/accumulo/fate/AdminUtil.java ---
@@ -110,25 +110,36 @@ public String getTop() {
   public static class FateStatus {
 
 private final List transactions;
-private final Map danglingHeldLocks;
-private final Map danglingWaitingLocks;
+private final Map danglingHeldLocks;
+private final Map danglingWaitingLocks;
+
+private static Map convert(Map 
danglocks) {
+  if (danglocks.isEmpty()) {
+return Collections.emptyMap();
+  }
+
+  Map ret = new HashMap<>();
+  for (Entry entry : danglocks.entrySet()) {
+ret.put(String.format("%016x", entry.getKey()), 
Collections.unmodifiableList(entry.getValue()));
--- End diff --

That's fine. This is more clear in the previous code when the formatting 
was in the log message. As a private utility method, it's far less clear how 
the formatting is used, or why it matters. It's not clear, for example, that 
this is just pre-formatting the object for future logging. The method is making 
assumptions about the caller's behavior.  It's probably fine, but it'd help if 
there were a comment explaining 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #209: ACCUMULO-4578 release namespace lock when compac...

[keith-turner]

Github user keith-turner commented on a diff in the pull request:

https://github.com/apache/accumulo/pull/209#discussion_r98488654
  
--- Diff: fate/src/main/java/org/apache/accumulo/fate/AdminUtil.java ---
@@ -110,25 +110,36 @@ public String getTop() {
   public static class FateStatus {
 
 private final List transactions;
-private final Map danglingHeldLocks;
-private final Map danglingWaitingLocks;
+private final Map danglingHeldLocks;
+private final Map danglingWaitingLocks;
+
+private static Map convert(Map 
danglocks) {
+  if (danglocks.isEmpty()) {
+return Collections.emptyMap();
+  }
+
+  Map ret = new HashMap<>();
+  for (Entry entry : danglocks.entrySet()) {
+ret.put(String.format("%016x", entry.getKey()), 
Collections.unmodifiableList(entry.getValue()));
--- End diff --

That is the format FATE uses when printing and logging transaction ids.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #209: ACCUMULO-4578 release namespace lock when compac...

[ctubbsii]

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

https://github.com/apache/accumulo/pull/209#discussion_r98486789
  
--- Diff: fate/src/main/java/org/apache/accumulo/fate/AdminUtil.java ---
@@ -110,25 +110,36 @@ public String getTop() {
   public static class FateStatus {
 
 private final List transactions;
-private final Map danglingHeldLocks;
-private final Map danglingWaitingLocks;
+private final Map danglingHeldLocks;
+private final Map danglingWaitingLocks;
+
+private static Map convert(Map 
danglocks) {
+  if (danglocks.isEmpty()) {
+return Collections.emptyMap();
+  }
+
+  Map ret = new HashMap<>();
+  for (Entry entry : danglocks.entrySet()) {
+ret.put(String.format("%016x", entry.getKey()), 
Collections.unmodifiableList(entry.getValue()));
--- End diff --

Why this particular string format?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #209: ACCUMULO-4578 release namespace lock when compac...

[ctubbsii]

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

https://github.com/apache/accumulo/pull/209#discussion_r98486517
  
--- Diff: fate/src/main/java/org/apache/accumulo/fate/AdminUtil.java ---
@@ -110,25 +110,36 @@ public String getTop() {
   public static class FateStatus {
 
 private final List transactions;
-private final Map danglingHeldLocks;
-private final Map danglingWaitingLocks;
+private final Map danglingHeldLocks;
+private final Map danglingWaitingLocks;
+
+private static Map convert(Map 
danglocks) {
--- End diff --

Private method needs a better name and/or docs. Convert what-to-what?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #204: ACCUMULO-4574 Modified TableOperations online to...

[keith-turner]

Github user keith-turner commented on a diff in the pull request:

https://github.com/apache/accumulo/pull/204#discussion_r98457211
  
--- Diff: 
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java
 ---
@@ -1208,11 +1207,20 @@ public void online(String tableName) throws 
AccumuloSecurityException, AccumuloE
   @Override
   public void online(String tableName, boolean wait) throws 
AccumuloSecurityException, AccumuloException, TableNotFoundException {
 checkArgument(tableName != null, "tableName is null");
+
+long cacheResetCount = Tables.getCacheResetCount();
+
 String tableId = Tables.getTableId(context.getInstance(), tableName);
 
 /**
  * ACCUMULO-4574 if table is already online return without executing 
fate operation.
  */
+
+// getTableId may have side effect of clearing zookeeper cache - no 
need to clear again.
+if (cacheResetCount == Tables.getCacheResetCount()) {
+  Tables.clearCacheByPath(context.getInstance(), Constants.ZTABLES + 
"/" + tableId + Constants.ZTABLE_STATE);
--- End diff --

Might be nice to centralize the zookeeper path construction in Tables 
class.   Instead of constructing the path here, could have a method called 
`Tables.clearCachedTableState(String tableId)`.  Alternatively, could add a 
boolean to  indicates if cache should be used`Tables.getTableState(String 
tableId, boolean useCache)`.  Structuring the code this way makes it easier to 
use this functionality elsewhere.  If I wanted to use what you have here 
elsewhere, I would probably copy and paste the path construction code you have 
here.  

I think I like adding the boolean to `getTableState` because that makes it 
easy to change the implementation in the future to not even use zoocache (just 
go straight to zookeeper) when the boolean is false.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #204: ACCUMULO-4574 Modified TableOperations online to...

[keith-turner]

Github user keith-turner commented on a diff in the pull request:

https://github.com/apache/accumulo/pull/204#discussion_r98455152
  
--- Diff: 
core/src/main/java/org/apache/accumulo/core/client/impl/Tables.java ---
@@ -149,6 +149,28 @@ public static void clearCache(Instance instance) {
 getZooCache(instance).clear(ZooUtil.getRoot(instance) + 
Constants.ZNAMESPACES);
   }
 
+  /**
+   * Clears the zoo cache from instance/root/{PATH}
+   *
+   * @param instance
+   *  The Accumulo Instance
+   * @param zooPath
+   *  A zookeeper path
+   */
+  public static void clearCacheByPath(Instance instance, final String 
zooPath) {
+
+String thePath;
+
+if (zooPath.startsWith("/")) {
+  thePath = zooPath;
+} else {
+  thePath = "/" + zooPath;
+}
+
+getZooCache(instance).clear(ZooUtil.getRoot(instance) + thePath);
--- End diff --

Do you know if this zoocache method handles path normalization?  For 
example will is treat `/a/b/c/` and `/a//b/c/` the same?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo issue #207: ACCUMULO-4446 Making changes to 1.7

[mjwall]

Github user mjwall commented on the issue:

https://github.com/apache/accumulo/pull/207
  
Looks like a jenkins issue.  I relaunched, but jenkins couldn't pull the
commit.  Not really sure what happened.  Can you push another commit to see
if runs again?

On Mon, Jan 30, 2017 at 8:55 AM lstav  wrote:

> No sure what happened to the Jenkins checks, it failed before the Travis
> CI even finished, can someone help me find out what the problem was?
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> , or 
mute
> the thread
> 

> .
>



---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo issue #207: ACCUMULO-4446 Making changes to 1.7

[lstav]

Github user lstav commented on the issue:

https://github.com/apache/accumulo/pull/207
  
No sure what happened to the Jenkins checks, it failed before the Travis CI 
even finished, can someone help me find out what the problem was?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #207: ACCUMULO-4446 Making changes to 1.7

[lstav]

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

https://github.com/apache/accumulo/pull/207#discussion_r98439532
  
--- Diff: 
server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java ---
@@ -642,7 +643,7 @@ private void getMonitorLock() throws KeeperException, 
InterruptedException {
   
UtilWaitThread.sleep(getContext().getConfiguration().getTimeInMillis(Property.MONITOR_LOCK_CHECK_INTERVAL));
 }
 
-log.info("Got Monitor lock.");
+log.info("Acquired Monitor Lock " + monitorLock.getLockPath());
--- End diff --

Thanks, just pushed the changes with the new format.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #207: ACCUMULO-4446 Making changes to 1.7

[lstav]

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

https://github.com/apache/accumulo/pull/207#discussion_r98432993
  
--- Diff: 
server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java ---
@@ -642,7 +643,7 @@ private void getMonitorLock() throws KeeperException, 
InterruptedException {
   
UtilWaitThread.sleep(getContext().getConfiguration().getTimeInMillis(Property.MONITOR_LOCK_CHECK_INTERVAL));
 }
 
-log.info("Got Monitor lock.");
+log.info("Acquired Monitor Lock " + monitorLock.getLockPath());
--- End diff --

I see, thanks for the reply!


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #207: ACCUMULO-4446 Making changes to 1.7

[EdColeman]

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

https://github.com/apache/accumulo/pull/207#discussion_r98433091
  
--- Diff: 
server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java ---
@@ -642,7 +643,7 @@ private void getMonitorLock() throws KeeperException, 
InterruptedException {
   
UtilWaitThread.sleep(getContext().getConfiguration().getTimeInMillis(Property.MONITOR_LOCK_CHECK_INTERVAL));
 }
 
-log.info("Got Monitor lock.");
+log.info("Acquired Monitor Lock " + monitorLock.getLockPath());
--- End diff --

Parameterized messages eliminate unnecessary object creation and we are 
moving to use that throughout the code base.  Without the parameter format, 
monitor.getLockPath() is always called, even if the log message will not be 
written. When passed as a parameter, the evaluation is deferred until the log 
statement has been determined that the value is going to be used.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #207: ACCUMULO-4446 Making changes to 1.7

[mjwall]

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

https://github.com/apache/accumulo/pull/207#discussion_r98432589
  
--- Diff: 
server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java ---
@@ -642,7 +643,7 @@ private void getMonitorLock() throws KeeperException, 
InterruptedException {
   
UtilWaitThread.sleep(getContext().getConfiguration().getTimeInMillis(Property.MONITOR_LOCK_CHECK_INTERVAL));
 }
 
-log.info("Got Monitor lock.");
+log.info("Acquired Monitor Lock " + monitorLock.getLockPath());
--- End diff --

@lstav check out https://www.slf4j.org/faq.html#logging_performance.  It's 
better because the string concatenation doesn't happen if the logger is not 
going to output.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] accumulo pull request #207: ACCUMULO-4446 Making changes to 1.7

[lstav]

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

https://github.com/apache/accumulo/pull/207#discussion_r98431979
  
--- Diff: 
server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java ---
@@ -642,7 +643,7 @@ private void getMonitorLock() throws KeeperException, 
InterruptedException {
   
UtilWaitThread.sleep(getContext().getConfiguration().getTimeInMillis(Property.MONITOR_LOCK_CHECK_INTERVAL));
 }
 
-log.info("Got Monitor lock.");
+log.info("Acquired Monitor Lock " + monitorLock.getLockPath());
--- End diff --

Any particular reason this would be better than what I used? I am not 
familiar with the slf4j format and I did it the way I did since some debug 
statements where written 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---