HADOOP-16830. Add public IOStatistics API + S3A implementation #1820
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
steveloughran
added
enhancement
fs/s3
steveloughran
changed the title
|
HADOOP-16830. Evolution of S3AStatisticsContext This is still at a -is this the right design?- stage. New class ActiveOperationContext. This is meant to be superclass of all operation context's, supplanting the S3AOpContext. I've also made WriteOperationHelper a subclass of it -which is one thing I'm not so sure of. Better as a constructor param. Almost all production references to the S3AInstrumentation class now refer to a much more minimal S3AStatisticsContext. The goal there is to have one which increments op-specific counters as well as the FS counters/metrics; for now there's a relay to Sticking this up while I do other things briefly, its here to show my direction of change. Regarding the IOStats API, I want to move off always returning something to making it Optional<>; that way callers can see its not there, but don't Next Steps
We inevitably will need to pass it around as an argument to every single innerXYX and other internal operation in S3AFS. Oops. It is critical, therefore, that ActiveOperationContext is something we don't find we need to replace in future; something we can just extend with things like
A WriteOperationContext will be a subclass with
Testing.All the intermittently flaky tests failed for me. I'm going to do an aggregate "fix the flaky's" patch |
steveloughran
force-pushed
the
s3/HADOOP-16830-iostatistics
branch
from
de0066b
to
c1b83b4
Compare
|
Checkstyle is all about unread stats names; I need to make sure they are in use in S3A then change checkstyle to not worry about that class |
steveloughran
force-pushed
the
s3/HADOOP-16830-iostatistics
branch
2 times, most recently
from
738d334
to
f035eae
Compare
|
steveloughran
force-pushed
the
s3/HADOOP-16830-iostatistics
branch
from
a85aeef
to
99c8f99
Compare
steveloughran
force-pushed
the
s3/HADOOP-16830-iostatistics
branch
from
5a30a82
to
3d43db4
Compare
|
s3a test failures are all mock-related. sigh |
steveloughran
force-pushed
the
s3/HADOOP-16830-iostatistics
branch
from
3d43db4
to
3fd535e
Compare
steveloughran
force-pushed
the
s3/HADOOP-16830-iostatistics
branch
4 times, most recently
from
c531518
to
5e0f2b3
Compare
steveloughran
force-pushed
the
s3/HADOOP-16830-iostatistics
branch
from
5e0f2b3
to
9738649
Compare
the change to the builder API needed to wire up AWS stats collection is broken regarding region signing calculation: need to understand more; may want to do an SDK updated too |
|
I've been convinced that we should just move to long off volatile for the stream statistics
Conclusion: we go with long and document in the IOStatistics API That it is better to be fast than synchronized if given a choice. |
|
|
||
| /** | ||
| * Probe for an attribute of this statistics set. | ||
| * @return attributes. |
| /** | ||
| * Treemaps sort their insertions so the iterator is ordered. | ||
| */ | ||
| private final Map<String, ToLongFunction<String>> evaluators |
There was a problem hiding this comment.
What is the intent of ToLong function here? Why can't we use Long directly?
There was a problem hiding this comment.
This is the "Dynamic" stats source -you build it with functions and whenever that stat is retrieved, it's re-evaluated. In implementation's I'm providing getters to values, with the standard one being a get() on an atomic long.
every time you look up a statistic, you get the newest value
| /** | ||
| * Support for working with statistics. | ||
| */ | ||
| public final class IOStatisticsSupport { |
There was a problem hiding this comment.
Duplicate class. Already present in impl.
There was a problem hiding this comment.
I'll rename one "IOStatisticsImplementationSupport"...the other is for app use
| /** | ||
| * Add a new evaluator to the statistics being built up. | ||
| * @param key key of this statistic | ||
| * @param source atomic counter |
There was a problem hiding this comment.
nit: atomic long counter.
| /** | ||
| * Add a new evaluator to the statistics being built up. | ||
| * @param key key of this statistic | ||
| * @param source atomic counter |
There was a problem hiding this comment.
nit: atomic integer counter. Since this is the only difference , I think we should mention.
| assertThat(iterator.hasNext()) | ||
| .describedAs("iterator.hasNext()") | ||
| .isFalse(); | ||
| assertThatThrownBy(iterator::next); |
There was a problem hiding this comment.
Should be asserting NoSuchElementException()
| assertStatisticUntracked(stats, "anything"); | ||
|
|
||
| // These actually test the assertion coverage | ||
| assertThatThrownBy(() -> |
There was a problem hiding this comment.
Do you mind explaining a bit more for this. Thanks
There was a problem hiding this comment.
will add javadocs & pull into own test cases
The S3A rework is somewhat independent, and I think we could look at to see what could be done to actually cut down the diff (I have a cunning plan). Change-Id: I5e72fb07af93506192f856d34c7591a1cf7d0ddb
Change-Id: I21fc73220de7ff3cf26bf3fde528c7b973878626
This adds stats collection from the AWS SDK. Failing as we can't set an endpoint on a builder-created s3 client instance, yet we need to use the builder to pass in the metrics. PITA Change-Id: Ia217e1151720b25e924f32558f821e7cfc7121a9
Change-Id: I3883fffa076f84cc0c4733bb1aa772a535095688
* just go for the simpler null/non-null API * writing unit tests and the helper assertions needed, initially just with an empty stats list Change-Id: I937562cba2facb663bf3097ed904a4d9147c8fdb TODO: support in local FS & tests
* tweak of the EmptyIOStatistics. We do need tests of the dynamic stuff next * S3A stats -findbugs changed to turn off complaints about volatile * AWS region binding code is all broken; landsat tests fail Change-Id: I734847cfed8979780377817560b818947768efc5
Adding first functional test for S3A stats. Broken :( Change-Id: I7d1a00f0fb8b7578dc9e1571b68af73c2f6320e7
steveloughran
force-pushed
the
s3/HADOOP-16830-iostatistics
branch
from
5fb699c
to
b81b389
Compare
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AInstrumentation.java
Show resolved
Hide resolved
Most of the work here is on getting the AWS Region binding code working again. I Don't understand it yet, but this mimics the old AWS SDK codepath. Test-wise: * block output stream counts bytes written as they are buffered to blocks; this is the statistic which is collected and examined. * network binding test not updated w.r.t changes in endpoint/region mapping Change-Id: Ia7392dd113772b5917c6990b966c1b7a28f38f20
* IOStatistics has a keys() method to return all the keys * moved S3A input stream the statistics to atomic longs (biggest change) * S3AInputStream no longer counts bite skipped during a seat within a buffer as read()...that is the bytes read counter is purely of bytes read by the application * renaming common statistic names so that they are more consistent Change-Id: Ica60840cf8a3c256cbb17fdf2ef79415df7ca7d6
Getting in the AWS metrics wired up is a lot harder Danny should be because the switch to the Builder API for the AWS S3 client seems to break most of the endpoint binding logic -this surfaces in failed tests which explicitly look at the data in US West two and elsewhere. We could fix the tests -but as they document a regression, it is better to do what we can to make the problem go away. Change-Id: I283aa6ad125df052f4b1dfca6308931ab7441a4c
Change-Id: I4ae3e39043ac5eddd4883fb1be846c43ae0d58b1
|
@mukund-thakur thanks for the review -tried to address your comments. Look in the package-info for some docs on how I imagine these to be used. Because the code doesn't compile, I'm going to rebase on trunk and submit that as a new PR That PR will revert back to the current S3A code to create an S3 client, because the builder mechanism needed to wire up the AWS metrics collection in the SDK is failing some of the tests -it is complex enough that it's slowing down the rest of the patch. We can add that later. |
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Statistics APIs and s3a stream implementation.
https://issues.apache.org/jira/browse/HADOOP-16830
Any FSInputStream subclass which is implements IOStatisticsSource will
have its statistics logged in a toString(), which makes for easy logging
of stats in downstream code.
Change-Id: If9a9b2396229088886f1b41cd5417ee78206aea8