[GitHub] accumulo pull request #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

[GitHub] accumulo pull request #270: ACCUMULO-4657 Reduce excessive logging / logging...

joshelser
GitHub user matthpeterson opened a pull request:

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

    ACCUMULO-4657 Reduce excessive logging / logging checks for Bulk Import code

   

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

    $ git pull https://github.com/matthpeterson/accumulo patch-1

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

    https://github.com/apache/accumulo/pull/270.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 #270
   
----
commit 84e815786f78c9391962ffd788265946abe546d2
Author: matthpeterson <[hidden email]>
Date:   2017-06-16T19:51:38Z

    Update MetadataTableUtil.java
   
    Outputting every "loaded" entry in the table is excessive, especially for tables with multiple simultaneous bulk imports and multiple references to the same file.  This has been seen to cause performance problems.  Even when the log level was reduced, there was blocking within log4j.  By doing that check once outside the loop and only logging at trace level, we have seen bulk import performance improvements.

----


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

    https://github.com/apache/accumulo/pull/270#discussion_r122793218
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +894,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      String tidString = Long.toString(tid);
           for (Entry<Key,Value> entry : mscanner) {
    -        log.debug("Looking at entry " + entry + " with tid " + tid);
    -        if (Long.parseLong(entry.getValue().toString()) == tid) {
    -          log.debug("deleting entry " + entry);
    -          Mutation m = new Mutation(entry.getKey().getRow());
    -          m.putDelete(entry.getKey().getColumnFamily(), entry.getKey().getColumnQualifier());
    +        if (shouldTrace) {
    +          log.trace("Looking at entry " + entry + " with tid " + tidString);
    --- End diff --
   
    We are using SLF4J in this class, right? You should be able to use the message syntax with object place holders here.


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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/270#discussion_r122803187
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +894,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      String tidString = Long.toString(tid);
    --- End diff --
   
    This is a nice optimization.  Could take it a step further and convert the string to a byte array and compare (using Arrays class) byte arrays in the loop.  This avoids converting each vals byte array to a string in the loop.


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

    https://github.com/apache/accumulo/pull/270#discussion_r122957684
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +894,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      String tidString = Long.toString(tid);
           for (Entry<Key,Value> entry : mscanner) {
    -        log.debug("Looking at entry " + entry + " with tid " + tid);
    -        if (Long.parseLong(entry.getValue().toString()) == tid) {
    -          log.debug("deleting entry " + entry);
    -          Mutation m = new Mutation(entry.getKey().getRow());
    -          m.putDelete(entry.getKey().getColumnFamily(), entry.getKey().getColumnQualifier());
    +        if (shouldTrace) {
    +          log.trace("Looking at entry " + entry + " with tid " + tidString);
    --- End diff --
   
    I don't think it buys us anything to use that syntax in this case.  I would still want to check the log level outside the loop to avoid calling the logger for every "loaded" entry.  The logger calls were where I saw the blocking, even when the log level was at INFO.


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

    https://github.com/apache/accumulo/pull/270#discussion_r122958951
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +894,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      String tidString = Long.toString(tid);
    --- End diff --
   
    Good idea.  Updated


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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/270#discussion_r122960232
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +895,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      byte[] tidAsBytes = Long.toString(tid).getBytes();
    --- End diff --
   
    For predictability in different environments, should call `getBytes(StandardCharsets.UTF_8)`.  Value's toString method did something like this in the prev code.


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

    https://github.com/apache/accumulo/pull/270#discussion_r122974876
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +895,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      byte[] tidAsBytes = Long.toString(tid).getBytes(UTF_8);
           for (Entry<Key,Value> entry : mscanner) {
    -        log.debug("Looking at entry " + entry + " with tid " + tid);
    -        if (Long.parseLong(entry.getValue().toString()) == tid) {
    -          log.debug("deleting entry " + entry);
    -          Mutation m = new Mutation(entry.getKey().getRow());
    -          m.putDelete(entry.getKey().getColumnFamily(), entry.getKey().getColumnQualifier());
    +        if (shouldTrace) {
    +          log.trace("Looking at entry " + entry + " with tid " + tid);
    --- End diff --
   
    log.trace("Looking at entry {} with tid {}", entry, tid);


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

    https://github.com/apache/accumulo/pull/270#discussion_r122961060
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +895,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      byte[] tidAsBytes = Long.toString(tid).getBytes();
    --- End diff --
   
    Updated, 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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

    https://github.com/apache/accumulo/pull/270#discussion_r123011175
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +895,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      byte[] tidAsBytes = Long.toString(tid).getBytes(UTF_8);
           for (Entry<Key,Value> entry : mscanner) {
    -        log.debug("Looking at entry " + entry + " with tid " + tid);
    -        if (Long.parseLong(entry.getValue().toString()) == tid) {
    -          log.debug("deleting entry " + entry);
    -          Mutation m = new Mutation(entry.getKey().getRow());
    -          m.putDelete(entry.getKey().getColumnFamily(), entry.getKey().getColumnQualifier());
    +        if (shouldTrace) {
    +          log.trace("Looking at entry " + entry + " with tid " + tid);
    --- End diff --
   
    I'm fine with that syntax but I'd still want the shouldTrace check:
   
    > I would still want to check the log level outside the loop to avoid calling the logger for every "loaded" entry. The logger calls were where I saw the blocking, even when the log level was at INFO.


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

    https://github.com/apache/accumulo/pull/270#discussion_r123020151
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +895,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      byte[] tidAsBytes = Long.toString(tid).getBytes(UTF_8);
           for (Entry<Key,Value> entry : mscanner) {
    -        log.debug("Looking at entry " + entry + " with tid " + tid);
    -        if (Long.parseLong(entry.getValue().toString()) == tid) {
    -          log.debug("deleting entry " + entry);
    -          Mutation m = new Mutation(entry.getKey().getRow());
    -          m.putDelete(entry.getKey().getColumnFamily(), entry.getKey().getColumnQualifier());
    +        if (shouldTrace) {
    +          log.trace("Looking at entry " + entry + " with tid " + tid);
    --- End diff --
   
    Fair enough - just realize it's being checked twice.
   
    https://github.com/qos-ch/slf4j/blob/master/slf4j-log4j12/src/main/java/org/slf4j/impl/Log4jLoggerAdapter.java#L175



---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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/270#discussion_r123024515
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +895,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      byte[] tidAsBytes = Long.toString(tid).getBytes(UTF_8);
           for (Entry<Key,Value> entry : mscanner) {
    -        log.debug("Looking at entry " + entry + " with tid " + tid);
    -        if (Long.parseLong(entry.getValue().toString()) == tid) {
    -          log.debug("deleting entry " + entry);
    -          Mutation m = new Mutation(entry.getKey().getRow());
    -          m.putDelete(entry.getKey().getColumnFamily(), entry.getKey().getColumnQualifier());
    +        if (shouldTrace) {
    +          log.trace("Looking at entry " + entry + " with tid " + tid);
    --- End diff --
   
    This is not related to this PR, but its something I learned about slf4j vs log4j.  This conversation reminded me of it and I thought it was worth sharing.
   
    Varargs methods can be expensive in a tight loop, because it allocates an array and copies to it.  Slf4j avoids this for one and two arguments by providing specialized methods.  However 3 or more args will use varargs.  
   
    I learned from @EdColeman that log4j V2  provides specialized methods for up to 10 arguments to avoid varargs. For example this is the [debug method that takes 10 args](https://logging.apache.org/log4j/2.x/log4j-api/apidocs/org/apache/logging/log4j/Logger.html#debug-org.apache.logging.log4j.Marker-java.lang.String-java.lang.Object-java.lang.Object-java.lang.Object-java.lang.Object-java.lang.Object-java.lang.Object-java.lang.Object-java.lang.Object-java.lang.Object-java.lang.Object-)
   
    Wish slf4j had this.


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging checks...

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

    https://github.com/apache/accumulo/pull/270
 
    Nice improvement. Do we not want these on 1.7 as well?


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

    https://github.com/apache/accumulo/pull/270#discussion_r123030301
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +894,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      String tidString = Long.toString(tid);
           for (Entry<Key,Value> entry : mscanner) {
    -        log.debug("Looking at entry " + entry + " with tid " + tid);
    -        if (Long.parseLong(entry.getValue().toString()) == tid) {
    -          log.debug("deleting entry " + entry);
    -          Mutation m = new Mutation(entry.getKey().getRow());
    -          m.putDelete(entry.getKey().getColumnFamily(), entry.getKey().getColumnQualifier());
    +        if (shouldTrace) {
    +          log.trace("Looking at entry " + entry + " with tid " + tidString);
    --- End diff --
   
    Where was that blocking? The problem of using the log(string) is that it calls the forced logger, whereas using the message syntax as @dlmarion  should bypass the synchronized check in the log4j category class ( if I recall correctly ). Can you double check 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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

    https://github.com/apache/accumulo/pull/270#discussion_r123045425
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +894,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      String tidString = Long.toString(tid);
           for (Entry<Key,Value> entry : mscanner) {
    -        log.debug("Looking at entry " + entry + " with tid " + tid);
    -        if (Long.parseLong(entry.getValue().toString()) == tid) {
    -          log.debug("deleting entry " + entry);
    -          Mutation m = new Mutation(entry.getKey().getRow());
    -          m.putDelete(entry.getKey().getColumnFamily(), entry.getKey().getColumnQualifier());
    +        if (shouldTrace) {
    +          log.trace("Looking at entry " + entry + " with tid " + tidString);
    --- End diff --
   
    Yes it was related to the forced logger:
    org.apache.log4j.Category.callAppenders(Category:204)
    waiting for lock .. (a org.apache.log4j.Logger)
    ..Category.forcedLog..
    ..Category.debug..
    ... removeBulkEntries(MetadataTableUtil.java:911)
   
    Yes, it looks like the slf4j implementation I checked avoids the forceLog call and instead checks against the level which would not block.


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging checks...

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

    https://github.com/apache/accumulo/pull/270
 
    @joshelser thanks.  I'm not sure about the plans for the releases.  I was hoping to get it into 1.8+.


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

    https://github.com/apache/accumulo/pull/270#discussion_r123046678
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +894,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      String tidString = Long.toString(tid);
           for (Entry<Key,Value> entry : mscanner) {
    -        log.debug("Looking at entry " + entry + " with tid " + tid);
    -        if (Long.parseLong(entry.getValue().toString()) == tid) {
    -          log.debug("deleting entry " + entry);
    -          Mutation m = new Mutation(entry.getKey().getRow());
    -          m.putDelete(entry.getKey().getColumnFamily(), entry.getKey().getColumnQualifier());
    +        if (shouldTrace) {
    +          log.trace("Looking at entry " + entry + " with tid " + tidString);
    --- End diff --
   
    It's ultimately cleaner to keep the conditional in slf4j. Does this change actually provide benefit over using SLF4j with the better call?


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

    https://github.com/apache/accumulo/pull/270#discussion_r123046966
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +894,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      String tidString = Long.toString(tid);
           for (Entry<Key,Value> entry : mscanner) {
    -        log.debug("Looking at entry " + entry + " with tid " + tid);
    -        if (Long.parseLong(entry.getValue().toString()) == tid) {
    -          log.debug("deleting entry " + entry);
    -          Mutation m = new Mutation(entry.getKey().getRow());
    -          m.putDelete(entry.getKey().getColumnFamily(), entry.getKey().getColumnQualifier());
    +        if (shouldTrace) {
    +          log.trace("Looking at entry " + entry + " with tid " + tidString);
    --- End diff --
   
    The version i modified wasn't using SLF4j yet so I'm more confident using the approach I took given that I haven't tested whether SLF4j achieves the same result.


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging checks...

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

    https://github.com/apache/accumulo/pull/270
 
    merging this in now.  I'll start it in 1.7 and merge it up


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

    https://github.com/apache/accumulo/pull/270#discussion_r123071638
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +894,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      String tidString = Long.toString(tid);
           for (Entry<Key,Value> entry : mscanner) {
    -        log.debug("Looking at entry " + entry + " with tid " + tid);
    -        if (Long.parseLong(entry.getValue().toString()) == tid) {
    -          log.debug("deleting entry " + entry);
    -          Mutation m = new Mutation(entry.getKey().getRow());
    -          m.putDelete(entry.getKey().getColumnFamily(), entry.getKey().getColumnQualifier());
    +        if (shouldTrace) {
    +          log.trace("Looking at entry " + entry + " with tid " + tidString);
    --- End diff --
   
    That's a pretty common paradigm that's used in SLF4J. what do you mean by "Achieves the same result?" It should achieve the same result. If it does not then there is an issue with SLF4J, at which point I would agree with you. If it does achieve the same result and the conditional is unnecessary, then why does it exist?


---
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 #270: ACCUMULO-4657 Reduce excessive logging / logging...

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

    https://github.com/apache/accumulo/pull/270#discussion_r123073391
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/util/MetadataTableUtil.java ---
    @@ -894,12 +895,19 @@ public static void removeBulkLoadEntries(Connector conn, String tableId, long ti
             BatchWriter bw = conn.createBatchWriter(MetadataTable.NAME, new BatchWriterConfig())) {
           mscanner.setRange(new KeyExtent(tableId, null, null).toMetadataRange());
           mscanner.fetchColumnFamily(TabletsSection.BulkFileColumnFamily.NAME);
    +      boolean shouldTrace = log.isTraceEnabled();
    +      byte[] tidAsBytes = Long.toString(tid).getBytes(UTF_8);
           for (Entry<Key,Value> entry : mscanner) {
    -        log.debug("Looking at entry " + entry + " with tid " + tid);
    -        if (Long.parseLong(entry.getValue().toString()) == tid) {
    -          log.debug("deleting entry " + entry);
    -          Mutation m = new Mutation(entry.getKey().getRow());
    -          m.putDelete(entry.getKey().getColumnFamily(), entry.getKey().getColumnQualifier());
    +        if (shouldTrace) {
    --- End diff --
   
    unnecessary and this should be removed when you merge it @mjwall


---
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.
---
12
Loading...