ctubbsii commented on code in PR #3124:
URL: https://github.com/apache/accumulo/pull/3124#discussion_r1052513407
##########
core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java:
##########
@@ -390,4 +390,21 @@ default Stream<Entry<Key,Value>> stream() {
return StreamSupport.stream(this.spliterator(), false);
}
+ /**
+ * Set user data on the Scanner. This data will be added to server side
information and logs for
+ * correlation.
+ *
+ * @param userData meaningful data that can be used to correlate server side
information
+ * @since 3.0.0
+ */
+ void setUserData(String userData);
Review Comment:
Would calling this a "tag" help? Also, the javadoc should mention that this
should be unique for each scan, in order to be useful.
##########
assemble/conf/log4j2-service.properties:
##########
@@ -84,6 +99,11 @@ logger.accumulo.level = debug
#logger.audit.additivity = false
#logger.audit.appenderRef.audit.ref = AuditLogFiles
+logger.userdata.name = org.apache.accumulo.core.logging.ScanUserDataLogger
+logger.userdata.level = off
Review Comment:
If we don't want to use a class name, we can use a logger name like
`org.apache.accumulo.userdata` (or `org.apache.accumulo.tags`), similar to what
we did with the audit logs. We still include `org.apache.accumulo.` as a
namespace prefix, but we don't need to use an actual class name for the logger
name. It can be any meaningful name.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftScanner.java:
##########
@@ -118,7 +118,7 @@ public static boolean getBatchFromServer(ClientContext
context, Range range, Key
ScanState scanState = new ScanState(context, extent.tableId(),
authorizations, range,
fetchedColumns, size, serverSideIteratorList,
serverSideIteratorOptions, false,
Constants.SCANNER_DEFAULT_READAHEAD_THRESHOLD, null, batchTimeOut,
classLoaderContext,
- null, false);
+ null, false, null);
Review Comment:
Should we use an empty string, or some default data for here?
##########
assemble/conf/log4j2-service.properties:
##########
@@ -72,6 +72,21 @@ appender.monitor.name = MonitorLog
appender.monitor.filter.threshold.type = ThresholdFilter
appender.monitor.filter.threshold.level = warn
+#appender.userdata.type = RollingFile
+#appender.userdata.name = UserDataLogFiles
+#appender.userdata.fileName = ${filename}.scan-user-data
+#appender.userdata.filePattern =
${filename}-%d{yyyy-MM-dd}-%i.scan-user-data.gz
+#appender.userdata.layout.type = PatternLayout
+#appender.userdata.layout.pattern = %d{ISO8601} [%-8c{2}] %-5p: %m%n
+#appender.userdata.policies.type = Policies
+#appender.userdata.policies.time.type = TimeBasedTriggeringPolicy
+#appender.userdata.policies.time.interval = 1
+#appender.userdata.policies.time.modulate = true
+#appender.userdata.policies.size.type = SizeBasedTriggeringPolicy
+#appender.userdata.policies.size.size=512MB
+#appender.userdata.strategy.type = DefaultRolloverStrategy
+#appender.userdata.strategy.max = 10
+
Review Comment:
In order to keep our example config file smaller, I don't think we need to
create a new appender for these. We can just include them by default in the
regular logs.
##########
core/src/main/java/org/apache/accumulo/core/logging/ScanUserDataLogger.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.Marker;
+import org.slf4j.event.Level;
+import org.slf4j.helpers.AbstractLogger;
+import org.slf4j.spi.LoggingEventBuilder;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class ScanUserDataLogger extends AbstractLogger {
Review Comment:
Until this PR, we've only ever depended upon `org.slf4j.Logger` and
`org.slf4j.LoggerFactory` from the SLF4J API. This has kept us in a very stable
position across SLF4J versions. This PR would depend upon other classes in the
SLF4J API, that I'm not sure are as stable. Given the importance of our use of
this dependency being widely compatible, because of its popularity as a
transitive dependency from a lot of our direct dependencies, I think we'd want
to try to find some other way to do this, without adding so much dependency on
other SLF4J APIs beyond the main two classes that we use today.
##########
core/src/main/java/org/apache/accumulo/core/logging/ScanUserDataLogger.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.Marker;
+import org.slf4j.event.Level;
+import org.slf4j.helpers.AbstractLogger;
+import org.slf4j.spi.LoggingEventBuilder;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class ScanUserDataLogger extends AbstractLogger {
+
+ private static final long serialVersionUID = 1L;
+ private static final Logger LOG =
LoggerFactory.getLogger(ScanUserDataLogger.class);
+ private static final ThreadLocal<String> USER_DATA = new ThreadLocal<>();
+
+ public static void logTrace(Logger classLogger, String userData, String msg,
Object... objects) {
+ if (classLogger != null) {
+ classLogger.trace(msg, objects);
+ }
+ if (!LOG.isTraceEnabled()) {
+ return;
+ }
+ USER_DATA.set(userData);
+ LOG.trace(msg, objects);
+ }
+
+ public static void logDebug(Logger classLogger, String userData, String msg,
Object... objects) {
+ if (classLogger != null) {
+ classLogger.debug(msg, objects);
+ }
+ if (!LOG.isDebugEnabled()) {
+ return;
+ }
+ USER_DATA.set(userData);
+ LOG.debug(msg, objects);
+ }
+
+ public static void logInfo(Logger classLogger, String userData, String msg,
Object... objects) {
+ if (classLogger != null) {
+ classLogger.info(msg, objects);
+ }
+ if (!LOG.isInfoEnabled()) {
+ return;
+ }
+ USER_DATA.set(userData);
+ LOG.info(msg, objects);
+ }
+
+ public static void logWarn(Logger classLogger, String userData, String msg,
Object... objects) {
+ if (classLogger != null) {
+ classLogger.warn(msg, objects);
+ }
+ if (!LOG.isWarnEnabled()) {
+ return;
+ }
+ USER_DATA.set(userData);
+ LOG.warn(msg, objects);
+ }
+
+ public static void logError(Logger classLogger, String userData, String msg,
Object... objects) {
+ if (classLogger != null) {
+ classLogger.error(msg, objects);
+ }
+ USER_DATA.set(userData);
+ LOG.error(msg, objects);
+ }
+
+ @Override
+ public boolean isTraceEnabled() {
+ return LOG.isTraceEnabled();
+ }
+
+ @Override
+ public boolean isTraceEnabled(Marker marker) {
+ return LOG.isTraceEnabled(marker);
+ }
+
+ @Override
+ public boolean isDebugEnabled() {
+ return LOG.isDebugEnabled();
+ }
+
+ @Override
+ public boolean isDebugEnabled(Marker marker) {
+ return LOG.isDebugEnabled(marker);
+ }
+
+ @Override
+ public boolean isInfoEnabled() {
+ return LOG.isInfoEnabled();
+ }
+
+ @Override
+ public boolean isInfoEnabled(Marker marker) {
+ return LOG.isInfoEnabled(marker);
+ }
+
+ @Override
+ public boolean isWarnEnabled() {
+ return LOG.isWarnEnabled();
+ }
+
+ @Override
+ public boolean isWarnEnabled(Marker marker) {
+ return LOG.isWarnEnabled(marker);
+ }
+
+ @Override
+ public boolean isErrorEnabled() {
+ return LOG.isErrorEnabled();
+ }
+
+ @Override
+ public boolean isErrorEnabled(Marker marker) {
+ return LOG.isErrorEnabled(marker);
+ }
+
+ @Override
+ protected String getFullyQualifiedCallerName() {
+ return null;
+ }
+
+ @Override
+ @SuppressFBWarnings(value = "RV_RETURN_VALUE_IGNORED",
+ justification = "Return value is self, using fluent object in non-fluent
manner")
Review Comment:
I'm assuming the warning being suppressed here is the builder, whose methods
return `this`.
This could be worded slightly better:
```suggestion
@SuppressFBWarnings(value = "RV_RETURN_VALUE_IGNORED",
justification = "fluent builder methods return itself, which we don't
need to use")
```
But, given that the implementation could change to have immutable
intermediates rather than return `this`, it would probably be better to just
use the builder API as intended.
##########
core/src/main/java/org/apache/accumulo/core/logging/ScanUserDataLogger.java:
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.Marker;
+import org.slf4j.event.Level;
+import org.slf4j.helpers.AbstractLogger;
+import org.slf4j.spi.LoggingEventBuilder;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class ScanUserDataLogger extends AbstractLogger {
+
+ private static final long serialVersionUID = 1L;
+ private static final Logger LOG =
LoggerFactory.getLogger(ScanUserDataLogger.class);
+ private static final ThreadLocal<String> USER_DATA = new ThreadLocal<>();
+
+ public static void logTrace(Logger classLogger, String userData, String msg,
Object... objects) {
+ if (classLogger != null) {
+ classLogger.trace(msg, objects);
+ }
+ if (!LOG.isTraceEnabled()) {
+ return;
+ }
+ USER_DATA.set(userData);
+ LOG.trace(msg, objects);
+ }
+
+ public static void logDebug(Logger classLogger, String userData, String msg,
Object... objects) {
+ if (classLogger != null) {
+ classLogger.debug(msg, objects);
+ }
+ if (!LOG.isDebugEnabled()) {
+ return;
+ }
+ USER_DATA.set(userData);
+ LOG.debug(msg, objects);
+ }
+
+ public static void logInfo(Logger classLogger, String userData, String msg,
Object... objects) {
+ if (classLogger != null) {
+ classLogger.info(msg, objects);
+ }
+ if (!LOG.isInfoEnabled()) {
+ return;
+ }
+ USER_DATA.set(userData);
+ LOG.info(msg, objects);
+ }
+
+ public static void logWarn(Logger classLogger, String userData, String msg,
Object... objects) {
+ if (classLogger != null) {
+ classLogger.warn(msg, objects);
+ }
+ if (!LOG.isWarnEnabled()) {
+ return;
+ }
+ USER_DATA.set(userData);
+ LOG.warn(msg, objects);
+ }
+
+ public static void logError(Logger classLogger, String userData, String msg,
Object... objects) {
+ if (classLogger != null) {
+ classLogger.error(msg, objects);
+ }
+ USER_DATA.set(userData);
+ LOG.error(msg, objects);
+ }
+
+ @Override
+ public boolean isTraceEnabled() {
+ return LOG.isTraceEnabled();
+ }
+
+ @Override
+ public boolean isTraceEnabled(Marker marker) {
+ return LOG.isTraceEnabled(marker);
+ }
+
+ @Override
+ public boolean isDebugEnabled() {
+ return LOG.isDebugEnabled();
+ }
+
+ @Override
+ public boolean isDebugEnabled(Marker marker) {
+ return LOG.isDebugEnabled(marker);
+ }
+
+ @Override
+ public boolean isInfoEnabled() {
+ return LOG.isInfoEnabled();
+ }
+
+ @Override
+ public boolean isInfoEnabled(Marker marker) {
+ return LOG.isInfoEnabled(marker);
+ }
+
+ @Override
+ public boolean isWarnEnabled() {
+ return LOG.isWarnEnabled();
+ }
+
+ @Override
+ public boolean isWarnEnabled(Marker marker) {
+ return LOG.isWarnEnabled(marker);
+ }
+
+ @Override
+ public boolean isErrorEnabled() {
+ return LOG.isErrorEnabled();
+ }
+
+ @Override
+ public boolean isErrorEnabled(Marker marker) {
+ return LOG.isErrorEnabled(marker);
+ }
+
+ @Override
+ protected String getFullyQualifiedCallerName() {
+ return null;
+ }
+
+ @Override
+ @SuppressFBWarnings(value = "RV_RETURN_VALUE_IGNORED",
+ justification = "Return value is self, using fluent object in non-fluent
manner")
+ protected void handleNormalizedLoggingCall(Level level, Marker marker,
String messagePattern,
+ Object[] arguments, Throwable throwable) {
+
+ final String userData = USER_DATA.get();
+ USER_DATA.set(null);
+
+ LoggingEventBuilder builder = LOG.atLevel(level);
+ if (marker != null) {
+ builder.addMarker(marker);
+ }
+ if (throwable != null) {
+ builder.setCause(throwable);
+ }
+ builder.setMessage("({}) " + messagePattern);
+ builder.addArgument(userData);
+ if (arguments != null) {
+ for (Object arg : arguments) {
+ builder.addArgument(arg);
+ }
+ }
Review Comment:
It's not clear the message pattern this is intending to construct. A simple
example in inline comments could help with debugging this.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ActiveScanIterator.java:
##########
@@ -71,9 +71,9 @@ private void readNext() {
final String header = String.format(
" %-21s| %-21s| %-9s| %-9s| %-7s| %-6s|"
- + " %-8s| %-8s| %-10s| %-20s| %-10s| %-10s | %-20s | %s",
+ + " %-8s| %-8s| %-10s| %-20s| %-10s| %-10s | %-20s | %s | %s",
Review Comment:
Because these are being printed... I'm wondering if it's better to use empty
strings instead of nulls wherever null is being passed for the default here.
This also makes me wonder if metadata scans internal to Accumulo should have a
unique, generated user data.
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/HintScanPrioritizer.java:
##########
@@ -80,6 +81,8 @@ private static int getPriority(ScanInfo si, int
defaultPriority, HintProblemActi
String prio = si.getExecutionHints().get("priority");
if (prio != null) {
try {
+ ScanUserDataLogger.logTrace(null, si.getUserData(),
Review Comment:
Why are we passing `null` for the logger? Under what circumstances would we
want to pass `null` vs. something else?
##########
core/src/main/java/org/apache/accumulo/core/spi/scan/ScanInfo.java:
##########
@@ -125,4 +125,9 @@ enum Type {
* @return Hints set by a scanner using {@link
ScannerBase#setExecutionHints(Map)}
*/
Map<String,String> getExecutionHints();
+
+ /**
+ * @return user data set by a scanner using {@link
ScannerBase#setUserData(String)}
+ */
+ String getUserData();
Review Comment:
Since this is adding a method to an API/SPI interface, it would be nice to
have a default method here, so it doesn't immediately break all users who
implemented this SPI in the previous release.
##########
shell/src/main/java/org/apache/accumulo/shell/commands/ScanCommand.java:
##########
@@ -95,6 +96,14 @@ protected ConsistencyLevel getConsistency(CommandLine cl) {
}
}
+ protected String getUserData(CommandLine cl) {
+ if (cl.hasOption(userDataOpt.getOpt())) {
+ return cl.getOptionValue(userDataOpt.getOpt());
+ } else {
+ return "Accumulo Shell";
Review Comment:
The shell could have a unique ID it uses by default here, so individual
shell instances can be distinguished from one another.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
