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 2020/10/20 20:25:39 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #1744: Use newer map methods in TabletGroupWatcher
milleruntime opened a new pull request #1744:
URL: https://github.com/apache/accumulo/pull/1744
----------------------------------------------------------------
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] milleruntime commented on a change in pull request #1744: Use newer map methods in TabletGroupWatcher
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1744:
URL: https://github.com/apache/accumulo/pull/1744#discussion_r509436204
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -214,14 +214,8 @@ public void run() {
TableId tableId = tls.extent.tableId();
TableConfiguration tableConf = this.master.getContext().getTableConfiguration(tableId);
- MergeStats mergeStats = mergeStatsCache.get(tableId);
- if (mergeStats == null) {
- mergeStats = currentMerges.get(tableId);
- if (mergeStats == null) {
- mergeStats = new MergeStats(new MergeInfo());
- }
- mergeStatsCache.put(tableId, mergeStats);
- }
+ MergeStats mergeStats = mergeStatsCache.computeIfAbsent(tableId,
+ k -> currentMerges.getOrDefault(k, new MergeStats(new MergeInfo())));
Review comment:
To clarify, this code will throw an exception even at the "info" level:
```
public class Slf4jLoggingTest {
private static Logger log = LoggerFactory.getLogger(Slf4jLoggingTest.class);
public static void main(String[] args) {
log.info("Start main");
log.debug("debug called: {}", expensiveCall());
log.info("Finished");
}
public static String expensiveCall() {
if (log.isInfoEnabled())
throw new UnsupportedOperationException("Do not call");
return "expensive string";
}
}
```
----------------------------------------------------------------
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] milleruntime commented on a change in pull request #1744: Use newer map methods in TabletGroupWatcher
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1744:
URL: https://github.com/apache/accumulo/pull/1744#discussion_r509436204
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -214,14 +214,8 @@ public void run() {
TableId tableId = tls.extent.tableId();
TableConfiguration tableConf = this.master.getContext().getTableConfiguration(tableId);
- MergeStats mergeStats = mergeStatsCache.get(tableId);
- if (mergeStats == null) {
- mergeStats = currentMerges.get(tableId);
- if (mergeStats == null) {
- mergeStats = new MergeStats(new MergeInfo());
- }
- mergeStatsCache.put(tableId, mergeStats);
- }
+ MergeStats mergeStats = mergeStatsCache.computeIfAbsent(tableId,
+ k -> currentMerges.getOrDefault(k, new MergeStats(new MergeInfo())));
Review comment:
To clarify, this code will throw an exception even at the "info" level:
```
public class Slf4jLoggingTest {
private static Logger log = LoggerFactory.getLogger(Slf4jLoggingTest.class);
public static void main(String[] args) {
log.info("Start main");
log.debug("debug called: {}", expensiveCall());
log.info("Finished");
}
public static String expensiveCall() {
if (true)
throw new UnsupportedOperationException("Do not call");
return "expensive string";
}
}
```
This code is actually passing a method but its the same idea. The method will get called to get the string it returns in the same way ```new Object()``` will get resolved.
----------------------------------------------------------------
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] milleruntime commented on a change in pull request #1744: Use newer map methods in TabletGroupWatcher
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1744:
URL: https://github.com/apache/accumulo/pull/1744#discussion_r509340450
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -214,14 +214,8 @@ public void run() {
TableId tableId = tls.extent.tableId();
TableConfiguration tableConf = this.master.getContext().getTableConfiguration(tableId);
- MergeStats mergeStats = mergeStatsCache.get(tableId);
- if (mergeStats == null) {
- mergeStats = currentMerges.get(tableId);
- if (mergeStats == null) {
- mergeStats = new MergeStats(new MergeInfo());
- }
- mergeStatsCache.put(tableId, mergeStats);
- }
+ MergeStats mergeStats = mergeStatsCache.computeIfAbsent(tableId,
+ k -> currentMerges.getOrDefault(k, new MergeStats(new MergeInfo())));
Review comment:
Now you got me thinking, how does ```map.getOrDefault(k, new MergeInfo())``` behave differently from ```log.debug("Merge: {}", new MergeInfo())```? Does slf4j have a special ability to not call the method on the interface based on logging level vs the default method on the Map interface, which will always execute the method?
----------------------------------------------------------------
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] milleruntime commented on a change in pull request #1744: Use newer map methods in TabletGroupWatcher
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1744:
URL: https://github.com/apache/accumulo/pull/1744#discussion_r509436204
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -214,14 +214,8 @@ public void run() {
TableId tableId = tls.extent.tableId();
TableConfiguration tableConf = this.master.getContext().getTableConfiguration(tableId);
- MergeStats mergeStats = mergeStatsCache.get(tableId);
- if (mergeStats == null) {
- mergeStats = currentMerges.get(tableId);
- if (mergeStats == null) {
- mergeStats = new MergeStats(new MergeInfo());
- }
- mergeStatsCache.put(tableId, mergeStats);
- }
+ MergeStats mergeStats = mergeStatsCache.computeIfAbsent(tableId,
+ k -> currentMerges.getOrDefault(k, new MergeStats(new MergeInfo())));
Review comment:
To clarify, this code will throw an exception even at the "info" level:
```
public class Slf4jLoggingTest {
private static Logger log = LoggerFactory.getLogger(Slf4jLoggingTest.class);
public static void main(String[] args) {
log.info("Start main");
log.debug("debug called: {}", expensiveCall());
log.info("Finished");
}
public static String expensiveCall() {
if (log.isInfoEnabled())
throw new UnsupportedOperationException("Do not call");
return "expensive string";
}
}
```
This code is actually passing a method but its the same idea. The method will get called to get the string it returns in the same way ```new Object()``` will get resolved.
----------------------------------------------------------------
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] milleruntime commented on a change in pull request #1744: Use newer map methods in TabletGroupWatcher
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1744:
URL: https://github.com/apache/accumulo/pull/1744#discussion_r509361089
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -214,14 +214,8 @@ public void run() {
TableId tableId = tls.extent.tableId();
TableConfiguration tableConf = this.master.getContext().getTableConfiguration(tableId);
- MergeStats mergeStats = mergeStatsCache.get(tableId);
- if (mergeStats == null) {
- mergeStats = currentMerges.get(tableId);
- if (mergeStats == null) {
- mergeStats = new MergeStats(new MergeInfo());
- }
- mergeStatsCache.put(tableId, mergeStats);
- }
+ MergeStats mergeStats = mergeStatsCache.computeIfAbsent(tableId,
+ k -> currentMerges.getOrDefault(k, new MergeStats(new MergeInfo())));
Review comment:
I guess I am confusing how sl4j doesn't create _unnecessary_ objects with the creation of an object when it is passed it as a parameter. If ```new Object()``` is passed as a parameter to a method, it will always create the object when the method is executed.
----------------------------------------------------------------
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] EdColeman commented on a change in pull request #1744: Use newer map methods in TabletGroupWatcher
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1744:
URL: https://github.com/apache/accumulo/pull/1744#discussion_r509441647
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -214,14 +214,8 @@ public void run() {
TableId tableId = tls.extent.tableId();
TableConfiguration tableConf = this.master.getContext().getTableConfiguration(tableId);
- MergeStats mergeStats = mergeStatsCache.get(tableId);
- if (mergeStats == null) {
- mergeStats = currentMerges.get(tableId);
- if (mergeStats == null) {
- mergeStats = new MergeStats(new MergeInfo());
- }
- mergeStatsCache.put(tableId, mergeStats);
- }
+ MergeStats mergeStats = mergeStatsCache.computeIfAbsent(tableId,
+ k -> currentMerges.getOrDefault(k, new MergeStats(new MergeInfo())));
Review comment:
With log4j 2 - it appears that lambdas can be used to provide lazy behavior.
`log.debug("debug called: {}", () -> expensiveCall());`
Would avoid the call if debug level logging was not enabled.
----------------------------------------------------------------
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 #1744: Use newer map methods in TabletGroupWatcher
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1744:
URL: https://github.com/apache/accumulo/pull/1744#discussion_r509600707
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -214,14 +214,8 @@ public void run() {
TableId tableId = tls.extent.tableId();
TableConfiguration tableConf = this.master.getContext().getTableConfiguration(tableId);
- MergeStats mergeStats = mergeStatsCache.get(tableId);
- if (mergeStats == null) {
- mergeStats = currentMerges.get(tableId);
- if (mergeStats == null) {
- mergeStats = new MergeStats(new MergeInfo());
- }
- mergeStatsCache.put(tableId, mergeStats);
- }
+ MergeStats mergeStats = mergeStatsCache.computeIfAbsent(tableId,
+ k -> currentMerges.getOrDefault(k, new MergeStats(new MergeInfo())));
Review comment:
Yeah, the same issue would apply to our slf4j debug statments... which is why we typically wrap stuff in an `if` block to check the log level `.isTraceEnabled()` first, to avoid making expensive objects.
----------------------------------------------------------------
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] milleruntime commented on a change in pull request #1744: Use newer map methods in TabletGroupWatcher
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1744:
URL: https://github.com/apache/accumulo/pull/1744#discussion_r509436204
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -214,14 +214,8 @@ public void run() {
TableId tableId = tls.extent.tableId();
TableConfiguration tableConf = this.master.getContext().getTableConfiguration(tableId);
- MergeStats mergeStats = mergeStatsCache.get(tableId);
- if (mergeStats == null) {
- mergeStats = currentMerges.get(tableId);
- if (mergeStats == null) {
- mergeStats = new MergeStats(new MergeInfo());
- }
- mergeStatsCache.put(tableId, mergeStats);
- }
+ MergeStats mergeStats = mergeStatsCache.computeIfAbsent(tableId,
+ k -> currentMerges.getOrDefault(k, new MergeStats(new MergeInfo())));
Review comment:
To clarify, this code will throw an exception even at the "info" level:
```
public static void main(String[] args) {
log.info("Start main");
log.debug("debug called: {}", expensiveCall());
log.info("Finished");
}
public static String expensiveCall() {
if (log.isInfoEnabled())
throw new UnsupportedOperationException("Do not call");
return "expensive string";
}
```
----------------------------------------------------------------
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 #1744: Use newer map methods in TabletGroupWatcher
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1744:
URL: https://github.com/apache/accumulo/pull/1744#discussion_r508861282
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -214,14 +214,8 @@ public void run() {
TableId tableId = tls.extent.tableId();
TableConfiguration tableConf = this.master.getContext().getTableConfiguration(tableId);
- MergeStats mergeStats = mergeStatsCache.get(tableId);
- if (mergeStats == null) {
- mergeStats = currentMerges.get(tableId);
- if (mergeStats == null) {
- mergeStats = new MergeStats(new MergeInfo());
- }
- mergeStatsCache.put(tableId, mergeStats);
- }
+ MergeStats mergeStats = mergeStatsCache.computeIfAbsent(tableId,
+ k -> currentMerges.getOrDefault(k, new MergeStats(new MergeInfo())));
Review comment:
This always constructs the the 'default' `new MergeStats(new MergeInfo()))`, even if already exists in `currentMerges`. To avoid that, you could use the following instead of `getOrDefault`:
```suggestion
k -> {
var mergeStats = currentMerges.get(k);
return mergeStats != null ? mergeStats : new MergeStats(new MergeInfo());
});
```
----------------------------------------------------------------
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] milleruntime commented on a change in pull request #1744: Use newer map methods in TabletGroupWatcher
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1744:
URL: https://github.com/apache/accumulo/pull/1744#discussion_r509448882
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -214,14 +214,8 @@ public void run() {
TableId tableId = tls.extent.tableId();
TableConfiguration tableConf = this.master.getContext().getTableConfiguration(tableId);
- MergeStats mergeStats = mergeStatsCache.get(tableId);
- if (mergeStats == null) {
- mergeStats = currentMerges.get(tableId);
- if (mergeStats == null) {
- mergeStats = new MergeStats(new MergeInfo());
- }
- mergeStatsCache.put(tableId, mergeStats);
- }
+ MergeStats mergeStats = mergeStatsCache.computeIfAbsent(tableId,
+ k -> currentMerges.getOrDefault(k, new MergeStats(new MergeInfo())));
Review comment:
This does work if you explicitly call the Log4j logger. Lambdas aren't implemented yet in slf4fj. There is an alpha version though: https://jira.qos.ch/browse/SLF4J-371
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -214,14 +214,8 @@ public void run() {
TableId tableId = tls.extent.tableId();
TableConfiguration tableConf = this.master.getContext().getTableConfiguration(tableId);
- MergeStats mergeStats = mergeStatsCache.get(tableId);
- if (mergeStats == null) {
- mergeStats = currentMerges.get(tableId);
- if (mergeStats == null) {
- mergeStats = new MergeStats(new MergeInfo());
- }
- mergeStatsCache.put(tableId, mergeStats);
- }
+ MergeStats mergeStats = mergeStatsCache.computeIfAbsent(tableId,
+ k -> currentMerges.getOrDefault(k, new MergeStats(new MergeInfo())));
Review comment:
This does work if you explicitly call the Log4j2 logger. Lambdas aren't implemented yet in slf4fj. There is an alpha version though: https://jira.qos.ch/browse/SLF4J-371
----------------------------------------------------------------
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] milleruntime merged pull request #1744: Use newer map methods in TabletGroupWatcher
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #1744:
URL: https://github.com/apache/accumulo/pull/1744
----------------------------------------------------------------
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