Allow user to set arbitrary data on Scanner to correlate with server side information #3124
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,7 @@ public class ActiveScanImpl extends ActiveScan { | |
| private Map<String,Map<String,String>> ssio; | ||
| private String user; | ||
| private Authorizations authorizations; | ||
| private String correlationId; | ||
|
Member
There was a problem hiding this comment. Honestly, I think
Contributor
There was a problem hiding this comment.
Another thought is if there is intent for a parent / child relationship then the name could reflect that.
Contributor
There was a problem hiding this comment. After reading correlationId I am more comfortable with correlationId because we can easily signal usage intent and reference other sources if additional clarification is needed.
Member
There was a problem hiding this comment. The problem with correlationId, is that it doesn't clue you in to the context of what it is correlating. I can imagine half a dozen different correlationIds in Accumulo... for scans, compactions, RPC requests in general, writes, bulk ingest, etc. They can't all be called correlationId internally. They need to be scoped somehow to the context that says more about what they are correlating. That context can be given by the interface the method is attached to, the class the field is in, or additional information in the log message, or some other way. But I think it's important to think about and provide that context somehow, if it isn't present, because "correlationId" by itself is just too generic of a concept that applies to too many contexts.
Contributor
There was a problem hiding this comment. If we migrate to using a specific logger like suggested, then the name of the logger could signal the context. In a general sense, the correlationId as a general concept looks to want to apply to a more general usage than just a scan. Say that a client reads, possibly updates and then writes the modified or new data within the scope of one unit of work for the client. When logged consistently, the correlationId could be used to tie all of the log statements for that client operation, scan writes,... across servers and processes. correlationId seems synonymous with transactionId in this context, without the implied atomic and rollback properties that are normally associated with transactions.
Member
There was a problem hiding this comment.
So I guess now I'm confused... which one are you going for here? I was under the impression you were going for a scan correlationId, but now I'm wondering if you are intending a client correlationId. If the latter, I would imagine we should focus on amending AccumuloClient.
Contributor
Author
There was a problem hiding this comment.
I was just saying that the same correlationId would be used by the client for those operations. I wasn't proposing a correlationId at the client.
Member
There was a problem hiding this comment.
I think I was proposing that, if that's the desired use case we're trying to support.
Contributor
There was a problem hiding this comment. I think a client correlationId should be the goal, this is a step that would not be incompatible with that.
Member
There was a problem hiding this comment.
I think adding the APIs to ScannerBase would need to be rolled back if we're going to make it client-wide. It doesn't make sense to add it there if we're going to change it to be added to AccumuloClient later. |
||
|
|
||
| ActiveScanImpl(ClientContext context, | ||
| org.apache.accumulo.core.tabletscan.thrift.ActiveScan activeScan) | ||
|
|
@@ -81,6 +82,7 @@ public class ActiveScanImpl extends ActiveScan { | |
| this.ssiList.add(ii.iterName + "=" + ii.priority + "," + ii.className); | ||
| } | ||
| this.ssio = activeScan.ssio; | ||
| this.correlationId = activeScan.correlationId; | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -152,4 +154,10 @@ public Authorizations getAuthorizations() { | |
| public long getIdleTime() { | ||
| return idle; | ||
| } | ||
|
|
||
| @Override | ||
| public String getCorrelationId() { | ||
| return correlationId; | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
| package org.apache.accumulo.core.logging; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.slf4j.event.Level; | ||
|
Member
There was a problem hiding this comment. I'm still a bit uncomfortable more tightly coupling ourselves to more SLF4J APIs, and have reservations about using Level here. Log4j's Level caused us a lot of problems removing it from our code, and although SLF4J's Level is maybe situated differently in its API, I can't help but anticipate the potential future problems we could have by using more of the logging framework's APIs in our code. I'd really prefer to avoid it. I also think that what I suggested before about having a logging lifecycle class, like what Keith did elsewhere would be better than adding this Logger class. I know Keith said that was out of scope for this PR... but I'm not so sure. If having that avoids this new class I think that's better than adding this class. If it needs to be done first, so it's a discrete task, that's fine... but I do think it's better than adding this class.
Contributor
There was a problem hiding this comment. I do agree with removing the extra sl4j new dependencies - I think the proposed solution of using Keith's method is what we should be targeting. As Christopher noted, the change in logging because of dependency changes took quite a while to unwind because we had basically painted ourselves into a corner, avoiding that happening again would be better. I think those changes could happen first, or this can be committed with the understanding that it would be reworked before any release. Separate PRs would make the review easier and either way would set-up merge conflicts with this PR. |
||
|
|
||
| public class CorrelationIdLogger { | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(CorrelationIdLogger.class); | ||
| private static final String FORMAT = "(%s) %s"; | ||
|
|
||
| /** | ||
| * Utility method that will log the msg and args to the calling class' logger (classLogger) if the | ||
| * classLogger argument is supplied. The classLogger argument would be null in the case where we | ||
| * don't want to log the same thing twice. | ||
| * | ||
| * @param level slf4j log level | ||
| * @param classLogger calling class' slf4j logger | ||
| * @param correlationId meaningful data that applications can use to correlate server side | ||
| * information | ||
| * @param msg log message | ||
| * @param args log message arguments | ||
| */ | ||
| public static void log(Level level, Logger classLogger, String correlationId, String msg, | ||
| Object... args) { | ||
| if (classLogger != null && classLogger.isEnabledForLevel(level)) { | ||
| classLogger.atLevel(level).log(msg, args); | ||
| } | ||
| if (LOG.isEnabledForLevel(level)) { | ||
| if (args == null) { | ||
| LOG.atLevel(level).log(String.format(FORMAT, correlationId, msg)); | ||
| } else { | ||
| LOG.atLevel(level).log(String.format(FORMAT, correlationId, msg), args); | ||
| } | ||
| } | ||
|
Comment on lines
+44
to
+53
Member
There was a problem hiding this comment. Having two log messages seems excessive. I think if the original logger had the necessary information, you can correlate the logs without logging a second time. I'm also thinking that the layers of formatting / interpolation here make this very confusing to understand/maintain. |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generically, "Correlation ID" is an ambiguous term, because it doesn't explain what two or more things it is correlating.
Calling it "Correlation ID" on the ScannerBase methods is fine, because the fact that it's on the Scanner API implies it's correlating scan information.
However, as a logger, we will need to ensure that log messages that use this CorrelationIdLogger include some context, so we know it's logging scan information. This could be a simple as (pseudo-code):
Logger("CorrelationIdLogger").log("Scan <id> ..."). The fact that it shows up in the logs as part of the CorrelationIdLogger tells us that it's a log message to correlate information about something. But, the fact that it says it's a "scan" in the message tells us that the correlation ID that is being provided is one associated with a scan.Presumably, other components than scans might also be able to log to the CorrelationIdLogger with their own context-specific IDs. If that's not the intent and this is intended only for scans, then it would be better to rename this logger to something that is scan-specific, and then we don't need to include "scan" in the message to add that important contextual information.