steveloughran commented on code in PR #4352:
URL: https://github.com/apache/hadoop/pull/4352#discussion_r917816203
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/statistics/impl/IOStatisticsContext.java:
##########
@@ -51,21 +51,21 @@ public class IOStatisticsContext {
private static final boolean IS_THREAD_IOSTATS_ENABLED;
private static final WeakReferenceThreadMap<IOStatisticsContext>
Review Comment:
let's add a brief comment here as it is a key part of the design
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/statistics/impl/IOStatisticsContext.java:
##########
@@ -164,13 +165,25 @@ public void setThreadIOStatistics(
*
* @return IOStatisticsSnapshot of the current thread.
*/
- public IOStatisticsSnapshot getThreadIOStatisticsSnapshot() {
+ public IOStatisticsSnapshot snapshotCurrentThreadIOStatistics() {
if (IS_THREAD_IOSTATS_ENABLED) {
return (IOStatisticsSnapshot) getThreadIOStatistics();
}
return new IOStatisticsSnapshot();
}
+ /**
+ * Reset the thread IOStatistics for current thread.
+ */
+ public void resetThreadIOStatisticsForCurrentThread() {
+ if (IS_THREAD_IOSTATS_ENABLED) {
+ IOStatisticsSnapshot currentThreadIOStatsSnapshot =
+ (IOStatisticsSnapshot) getThreadIOStatistics();
+ currentThreadIOStatsSnapshot.clear();
+ setThreadIOStatistics(currentThreadIOStatsSnapshot);
Review Comment:
don't think this is needed
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/statistics/impl/IOStatisticsContext.java:
##########
@@ -51,21 +51,21 @@ public class IOStatisticsContext {
private static final boolean IS_THREAD_IOSTATS_ENABLED;
private static final WeakReferenceThreadMap<IOStatisticsContext>
- ACTIVE_IOSTATS_CONTEXT = new WeakReferenceThreadMap<>(
- IOStatisticsContext::createNewInstance,
- IOStatisticsContext::referenceLostContext
+ ACTIVE_IOSTATS_CONTEXT =
+ new WeakReferenceThreadMap<>(IOStatisticsContext::createNewInstance,
+ IOStatisticsContext::referenceLostContext
);
/**
* Collecting IOStatistics per thread.
*/
private final WeakReferenceThreadMap<IOStatisticsAggregator>
Review Comment:
now we are casting to a snapshot, why not make this of type
`IOStatisticsSnapshot`
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/statistics/impl/IOStatisticsContext.java:
##########
@@ -164,13 +165,25 @@ public void setThreadIOStatistics(
*
* @return IOStatisticsSnapshot of the current thread.
*/
- public IOStatisticsSnapshot getThreadIOStatisticsSnapshot() {
+ public IOStatisticsSnapshot snapshotCurrentThreadIOStatistics() {
if (IS_THREAD_IOSTATS_ENABLED) {
return (IOStatisticsSnapshot) getThreadIOStatistics();
}
return new IOStatisticsSnapshot();
}
+ /**
+ * Reset the thread IOStatistics for current thread.
+ */
+ public void resetThreadIOStatisticsForCurrentThread() {
+ if (IS_THREAD_IOSTATS_ENABLED) {
+ IOStatisticsSnapshot currentThreadIOStatsSnapshot =
+ (IOStatisticsSnapshot) getThreadIOStatistics();
Review Comment:
This is going to demand create a snapshot context even if there wasn't one
needing resetting. How about you just look in the reference map (via
lookup(currentThreadId())); and only if a ref is returned do the resetting?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
