You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/07/11 11:26:34 UTC
[GitHub] [hadoop] steveloughran commented on a diff in pull request #4352: HADOOP-17461. Thread-level IOStatistics in S3A
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org