-
Notifications
You must be signed in to change notification settings - Fork 9.2k
Hadoop-19676: ABFS: Aggregated Metrics Manager with Multiple Emission Criteria #8137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Changes from all commits
4acc11d
18d2be9
abf1f1e
9111f5e
1af17c1
e6a6087
ba2961c
f371547
fb5accc
da8c392
c89af43
3f8af3d
fbd203f
655dcb1
2320a9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,6 +73,7 @@ | |
| import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.SEND_REQUESTS; | ||
| import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.SERVER_UNAVAILABLE; | ||
| import static org.apache.hadoop.fs.azurebfs.AbfsStatistic.WRITE_THROTTLES; | ||
| import static org.apache.hadoop.fs.azurebfs.constants.AbfsHttpConstants.EMPTY_STRING; | ||
| import static org.apache.hadoop.fs.azurebfs.enums.AbfsBackoffMetricsEnum.TOTAL_NUMBER_OF_REQUESTS; | ||
| import static org.apache.hadoop.fs.statistics.impl.IOStatisticsBinding.iostatisticsStore; | ||
| import static org.apache.hadoop.util.Time.now; | ||
|
|
@@ -201,23 +202,39 @@ public void initializeWriteResourceUtilizationMetrics() { | |
|
|
||
|
|
||
| @Override | ||
| public void initializeMetrics(MetricFormat metricFormat) { | ||
| public void initializeMetrics(final MetricFormat metricFormat, | ||
| final AbfsConfiguration abfsConfiguration) { | ||
| switch (metricFormat) { | ||
| case INTERNAL_BACKOFF_METRIC_FORMAT: | ||
| abfsBackoffMetrics = new AbfsBackoffMetrics(); | ||
| break; | ||
| case INTERNAL_FOOTER_METRIC_FORMAT: | ||
| abfsReadFooterMetrics = new AbfsReadFooterMetrics(); | ||
| break; | ||
| case INTERNAL_METRIC_FORMAT: | ||
| abfsBackoffMetrics = new AbfsBackoffMetrics(); | ||
| abfsReadFooterMetrics = new AbfsReadFooterMetrics(); | ||
| break; | ||
| default: | ||
| break; | ||
| case INTERNAL_BACKOFF_METRIC_FORMAT: | ||
| abfsBackoffMetrics = new AbfsBackoffMetrics( | ||
| abfsConfiguration.isBackoffRetryMetricsEnabled()); | ||
| break; | ||
| case INTERNAL_FOOTER_METRIC_FORMAT: | ||
| initializeReadFooterMetrics(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. break missing here |
||
| break; | ||
| case INTERNAL_METRIC_FORMAT: | ||
| abfsBackoffMetrics = new AbfsBackoffMetrics( | ||
| abfsConfiguration.isBackoffRetryMetricsEnabled()); | ||
| initializeReadFooterMetrics(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can do sm like we would have a single constructor for AbfsReadFooterMetrics then
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even then we may need two constructors as it is getting used in some test cases. But as you suggest I can simplify this if else statement in initializeReadFooterMetrics method. |
||
| break; | ||
| default: | ||
| break; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: break is redundant
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is as per best practice to keep break statement with default case. |
||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Initialize the read footer metrics. | ||
| * In case the metrics are already initialized, | ||
| * create a new instance with the existing map. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why create a new instance?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After the collected metrics are pushed to the backend, the counters are refreshed by creating new instances of the backoff and read-footer metrics rather than clearing each counter individually. The old instances are automatically garbage-collected. |
||
| */ | ||
| private void initializeReadFooterMetrics() { | ||
| abfsReadFooterMetrics = new AbfsReadFooterMetrics( | ||
| abfsReadFooterMetrics == null | ||
| ? null | ||
| : abfsReadFooterMetrics.getFileTypeMetricsMap() | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Look up a Metric from registered set. | ||
| * | ||
|
|
@@ -373,10 +390,9 @@ public DurationTracker trackDuration(String key) { | |
|
|
||
| @Override | ||
| public String toString() { | ||
| String metric = ""; | ||
| String metric = EMPTY_STRING; | ||
| if (abfsBackoffMetrics != null) { | ||
| long totalNoRequests = getAbfsBackoffMetrics().getMetricValue(TOTAL_NUMBER_OF_REQUESTS); | ||
| if (totalNoRequests > 0) { | ||
| if (getAbfsBackoffMetrics().getMetricValue(TOTAL_NUMBER_OF_REQUESTS) > 0) { | ||
| metric += "#BO:" + getAbfsBackoffMetrics().toString(); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,6 +136,13 @@ public final class FileSystemConfigurations { | |
| public static final boolean DEFAULT_ENABLE_AUTOTHROTTLING = false; | ||
| public static final int DEFAULT_METRIC_IDLE_TIMEOUT_MS = 60_000; | ||
| public static final int DEFAULT_METRIC_ANALYSIS_TIMEOUT_MS = 60_000; | ||
| public static final boolean DEFAULT_METRICS_COLLECTION_ENABLED = true; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want to keep it enabled by default for all Cxs?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this feature will be enabled by default. |
||
| public static final boolean DEFAULT_METRICS_SHOULD_EMIT_ON_IDLE_TIME = false; | ||
| public static final long DEFAULT_METRICS_EMIT_THRESHOLD = 100_000L; | ||
| public static final long DEFAULT_METRICS_EMIT_THRESHOLD_INTERVAL_SECS = 60; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Genrally names here also contains FS. Format was exact config variable name prefixed with DEFAULT_ I know this isn't followed everywhere but let's try to resolve this here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, earlier as well we keep it DEFAULT_. We can discuss it offline as it will make the variable name bigger.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed, no changes required here. |
||
| public static final long DEFAULT_METRICS_EMIT_INTERVAL_MINS = 60; | ||
| public static final int DEFAULT_METRICS_MAX_CALLS_PER_SECOND = 3; | ||
| public static final boolean DEFAULT_METRICS_BACKOFF_RETRY_ENABLED = false; | ||
| public static final boolean DEFAULT_FS_AZURE_ACCOUNT_LEVEL_THROTTLING_ENABLED = true; | ||
| public static final int DEFAULT_ACCOUNT_OPERATION_IDLE_TIMEOUT_MS = 60_000; | ||
| public static final int DEFAULT_ANALYSIS_PERIOD_MS = 10_000; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing AbfsConfiguration here, better to only pass isRetryMetricsEnabled.
Whole object is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for sending the entire AbfsConfiguration was that we may need additional configuration values in the future; in that case, we can fetch them directly without having to change this again.