keith-turner commented on code in PR #3124:
URL: https://github.com/apache/accumulo/pull/3124#discussion_r1048284855
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftScanClientHandler.java:
##########
@@ -147,34 +147,40 @@ public void close() {}
};
return this.startScan(tinfo, credentials, extent, range, columns,
batchSize, ssiList, ssio,
authorizations, waitForWrites, isolated, readaheadThreshold,
tSamplerConfig, batchTimeOut,
- contextArg, executionHints, resolver, busyTimeout);
+ contextArg, executionHints, resolver, busyTimeout, userData);
}
public InitialScan startScan(TInfo tinfo, TCredentials credentials,
KeyExtent extent,
TRange range, List<TColumn> columns, int batchSize, List<IterInfo>
ssiList,
Map<String,Map<String,String>> ssio, List<ByteBuffer> authorizations,
boolean waitForWrites,
boolean isolated, long readaheadThreshold, TSamplerConfiguration
tSamplerConfig,
long batchTimeOut, String contextArg, Map<String,String> executionHints,
- ScanSession.TabletResolver tabletResolver, long busyTimeout) throws
NotServingTabletException,
- ThriftSecurityException,
org.apache.accumulo.core.tabletserver.thrift.TooManyFilesException,
+ ScanSession.TabletResolver tabletResolver, long busyTimeout, String
userData)
+ throws NotServingTabletException, ThriftSecurityException,
+ org.apache.accumulo.core.tabletserver.thrift.TooManyFilesException,
TSampleNotPresentException, ScanServerBusyException {
+ log.debug("({}) starting scan", userData);
+
server.getScanMetrics().incrementStartScan(1.0D);
TableId tableId = extent.tableId();
NamespaceId namespaceId;
try {
namespaceId = server.getContext().getNamespaceId(tableId);
} catch (TableNotFoundException e1) {
+ log.error("({}) not serving tablet {}", userData, extent.toThrift());
Review Comment:
Thinking this would be better logged at debug or trace. Tables being
concurrently deleted while a scan is attempted can be a normal course of
business.
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftScanClientHandler.java:
##########
@@ -195,6 +201,7 @@ public InitialScan startScan(TInfo tinfo, TCredentials
credentials, KeyExtent ex
TabletBase tablet = tabletResolver.getTablet(extent);
if (tablet == null) {
+ log.error("({}) not serving tablet {}", userData, extent.toThrift());
Review Comment:
Thinking this should be logged at debug or trace, its not unexpected that
tablets move around.
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftScanClientHandler.java:
##########
@@ -147,34 +147,40 @@ public void close() {}
};
return this.startScan(tinfo, credentials, extent, range, columns,
batchSize, ssiList, ssio,
authorizations, waitForWrites, isolated, readaheadThreshold,
tSamplerConfig, batchTimeOut,
- contextArg, executionHints, resolver, busyTimeout);
+ contextArg, executionHints, resolver, busyTimeout, userData);
}
public InitialScan startScan(TInfo tinfo, TCredentials credentials,
KeyExtent extent,
TRange range, List<TColumn> columns, int batchSize, List<IterInfo>
ssiList,
Map<String,Map<String,String>> ssio, List<ByteBuffer> authorizations,
boolean waitForWrites,
boolean isolated, long readaheadThreshold, TSamplerConfiguration
tSamplerConfig,
long batchTimeOut, String contextArg, Map<String,String> executionHints,
- ScanSession.TabletResolver tabletResolver, long busyTimeout) throws
NotServingTabletException,
- ThriftSecurityException,
org.apache.accumulo.core.tabletserver.thrift.TooManyFilesException,
+ ScanSession.TabletResolver tabletResolver, long busyTimeout, String
userData)
+ throws NotServingTabletException, ThriftSecurityException,
+ org.apache.accumulo.core.tabletserver.thrift.TooManyFilesException,
TSampleNotPresentException, ScanServerBusyException {
+ log.debug("({}) starting scan", userData);
Review Comment:
Per scan session logging is very granular. I feel like it should be trace
instead of debug. I suspect this could contribute to a large percentage of the
debug logs.
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftScanClientHandler.java:
##########
@@ -147,34 +147,40 @@ public void close() {}
};
return this.startScan(tinfo, credentials, extent, range, columns,
batchSize, ssiList, ssio,
authorizations, waitForWrites, isolated, readaheadThreshold,
tSamplerConfig, batchTimeOut,
- contextArg, executionHints, resolver, busyTimeout);
+ contextArg, executionHints, resolver, busyTimeout, userData);
}
public InitialScan startScan(TInfo tinfo, TCredentials credentials,
KeyExtent extent,
TRange range, List<TColumn> columns, int batchSize, List<IterInfo>
ssiList,
Map<String,Map<String,String>> ssio, List<ByteBuffer> authorizations,
boolean waitForWrites,
boolean isolated, long readaheadThreshold, TSamplerConfiguration
tSamplerConfig,
long batchTimeOut, String contextArg, Map<String,String> executionHints,
- ScanSession.TabletResolver tabletResolver, long busyTimeout) throws
NotServingTabletException,
- ThriftSecurityException,
org.apache.accumulo.core.tabletserver.thrift.TooManyFilesException,
+ ScanSession.TabletResolver tabletResolver, long busyTimeout, String
userData)
+ throws NotServingTabletException, ThriftSecurityException,
+ org.apache.accumulo.core.tabletserver.thrift.TooManyFilesException,
TSampleNotPresentException, ScanServerBusyException {
+ log.debug("({}) starting scan", userData);
+
server.getScanMetrics().incrementStartScan(1.0D);
TableId tableId = extent.tableId();
NamespaceId namespaceId;
try {
namespaceId = server.getContext().getNamespaceId(tableId);
} catch (TableNotFoundException e1) {
+ log.error("({}) not serving tablet {}", userData, extent.toThrift());
throw new NotServingTabletException(extent.toThrift());
}
if (!security.canScan(credentials, tableId, namespaceId, range, columns,
ssiList, ssio,
authorizations)) {
+ log.error("({}) permission denied", userData);
Review Comment:
I am wondering if this duplicates what the audit logging is doing. If so
may be better to pass the user data to the audit logging. Also, not sure it
should be logged at error.
--
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]
