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 2022/12/14 10:50:26 UTC
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3124: Allow user to set arbitrary data on Scanner to correlate with server side information
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org