You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by lstav <gi...@git.apache.org> on 2017/01/26 14:39:34 UTC
[GitHub] accumulo pull request #207: ACCUMULO-4446 Making changes to 1.7
GitHub user lstav opened a pull request:
https://github.com/apache/accumulo/pull/207
ACCUMULO-4446 Making changes to 1.7
These changes are the same as those in #195
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lstav/accumulo ACCCUMULO-4446-1.7
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/207.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 #207
----
commit 23b7969a8ecf557ab5653fdc8f5075ddf251b157
Author: Luis Tavarez <ze...@outlook.com>
Date: 2017-01-26T14:35:40Z
ACCUMULO-4446 Making changes to 1.7
----
---
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 #207: ACCUMULO-4446 Making changes to 1.7
Posted by EdColeman <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #207: ACCUMULO-4446 Making changes to 1.7
Posted by EdColeman <gi...@git.apache.org>.
Github user EdColeman commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/207#discussion_r98139508
--- 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 --
Please consider using slf4j parameterized message format.
`log.info("Acquired Monitor Lock {}", monitorLock.getLockPath()); `
---
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 #207: ACCUMULO-4446 Making changes to 1.7
Posted by lstav <gi...@git.apache.org>.
Github user lstav commented on the issue:
https://github.com/apache/accumulo/pull/207
Sure, will do
---
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 #207: ACCUMULO-4446 Making changes to 1.7
Posted by lstav <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #207: ACCUMULO-4446 Making changes to 1.7
Posted by lstav <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #207: ACCUMULO-4446 Making changes to 1.7
Posted by lstav <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #207: ACCUMULO-4446 Making changes to 1.7
Posted by mjwall <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #207: ACCUMULO-4446 Making changes to 1.7
Posted by lstav <gi...@git.apache.org>.
Github user lstav closed the pull request at:
https://github.com/apache/accumulo/pull/207
---
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 #207: ACCUMULO-4446 Making changes to 1.7
Posted by mjwall <gi...@git.apache.org>.
Github user mjwall commented on the issue:
https://github.com/apache/accumulo/pull/207
merged in, please close this 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 issue #207: ACCUMULO-4446 Making changes to 1.7
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on the issue:
https://github.com/apache/accumulo/pull/207
I recommend a better description for the git commit log. Something which describes the nature of the change, rather than simply referring to the ticket number and branch.
---
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 #207: ACCUMULO-4446 Making changes to 1.7
Posted by mjwall <gi...@git.apache.org>.
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 <no...@github.com> 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?
>
> \u2014
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/accumulo/pull/207#issuecomment-276068019>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AAAJJvrh8vfkw8IekJ69CzfbMuPFoaYyks5rXevNgaJpZM4Lus-G>
> .
>
---
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 #207: ACCUMULO-4446 Making changes to 1.7
Posted by EdColeman <gi...@git.apache.org>.
Github user EdColeman commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/207#discussion_r98139545
--- Diff: server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java ---
@@ -2383,12 +2383,12 @@ public void run() {
for (int i = 0; i < 120 / 5; i++) {
zoo.putPersistentData(zPath, new byte[0], NodeExistsPolicy.SKIP);
+ log.info("Attempting to acquire Tablet Server Lock");
if (tabletServerLock.tryLock(lw, lockContent)) {
- log.debug("Obtained tablet server lock " + tabletServerLock.getLockPath());
+ log.info("Acquired Tablet Server Lock " + tabletServerLock.getLockPath());
--- End diff --
Please consider using slf4j parameterized message format.
` log.info("Acquired Tablet Server Lock {}", tabletServerLock.getLockPath()); `
---
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 #207: ACCUMULO-4446 Making changes to 1.7
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #207: ACCUMULO-4446 Making changes to 1.7
Posted by lstav <gi...@git.apache.org>.
Github user lstav commented on the issue:
https://github.com/apache/accumulo/pull/207
Just made the change for the commit message and added a description to 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 #207: ACCUMULO-4446 Making changes to 1.7
Posted by lstav <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---