Uh oh!
There was an error while loading. Please reload this page.
Backport "HBASE-26474 Implement connection-level attributes" to branch-2 - #4014
Conversation
ndimiduk
commented
Jan 6, 2022
reviewers: please notice this change to internal behavior of caching region locations in Previously, this method returned a I think the one-to-one mapping is an unnecessary preservation of arbitrary physical topology of the scan of the meta region. The change here has This new implementation may also make region caching slightly (negligibly) faster because the logic for cache entry management is quite complex, while the logic for naively combining multiple |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
joshelser
left a comment
There was a problem hiding this comment.
Barring the comments from Huaxiang, looks pretty reasonable to me. A couple questions on type-hierarchy and package visibility, but overall it looks like a very nice cleanup (around a tough-to-reason part of HBase code).
Uh oh!
There was an error while loading. Please reload this page.
| return Optional.ofNullable(location) | ||
| .map(HRegionLocation::getRegion) | ||
| .map(RegionInfo::getRegionNameAsString) | ||
| .map(Collections::singletonList) |
There was a problem hiding this comment.
This will net an immutable list whereas Arrays.asList() (as previously used down below) was returning a mutable list. Hopefully we would not be altering this List, but noting a change in implementation to make sure it was intentional.
There was a problem hiding this comment.
Noted. I had not considered mutability of the previous implementation. Consider this change to immutability a happy side-effect. At least on the master PR, this appears to have introduced no issues. Perhaps this is cause for some of the test failures I seem to have caused in the branch-2 backport. Let me investigate.
There was a problem hiding this comment.
No test failures after reverting the erroneous changes pointed out by Huaxiang, so I'll take that as acceptability of the immutable collection. Shout if that's not okay by you.
Uh oh!
There was an error while loading. Please reload this page.
| * All {@link Span}s involving {@code conn} should include these attributes. | ||
| * @see #buildConnectionAttributesMatcher(AsyncConnectionImpl) | ||
| */ | ||
| public static Matcher<SpanData> buildConnectionAttributesMatcher(ConnectionImplementation conn) { |
There was a problem hiding this comment.
Is this usage why ConnectionImplementation was made public? I remember when ConnectionImplementation was made package-private back in HBase 1.x. Do you think this is sufficient to make it public again? (or maybe I've missed the real reason)
There was a problem hiding this comment.
Yes, Both ConnectionImplementation and AsyncConnectionImpl became public classes in order to facilitate their introspection for tracing. Not for this test class, but for their use in the Builders. They remain IA.Private, so I'm not concerned about leaking to user applications. I'm not familiar with the conversation that lead to them being package private; do you have context or specific concerns that can enlighten me?
Uh oh!
There was an error while loading. Please reload this page.
| * Construct {@link Span} instances involving data tables. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public class TableSpanBuilder implements Supplier<Span> { |
There was a problem hiding this comment.
Pretty similar to ConnectionSpanBuilder. Should they share a parent class or some common utility methods?
There was a problem hiding this comment.
An earlier version of this code made use of inheritance for these builders and the template gymnastics required were awful. Following Duo's suggestion, I undid that implementation in favor of this mix-in style approach with reused utility methods. Lots of code that was DRY in the inheritance version becomes repeated in this mix-in style. For me, I think this repeated code is worth the hassle, because it'll be easier to maintain than the complex hierarchy with esoteric template definitions.
All that said, if you see specific segments that could be extracted into utility methods, I'm happy to make the improvements.
ndimiduk
left a comment
There was a problem hiding this comment.
Thank you @huaxiangsun for pointing out the error of my ways. Undoing those changes to region locations, this backport becomes boring. Let's see if the pre-commit tests agree with me.
Thank you @joshelser for your comments. I wish I'd had your attention on the master PR. I may need to put up an addendum to address one or two of your concerns.
Uh oh!
There was an error while loading. Please reload this page.
| return Optional.ofNullable(location) | ||
| .map(HRegionLocation::getRegion) | ||
| .map(RegionInfo::getRegionNameAsString) | ||
| .map(Collections::singletonList) |
There was a problem hiding this comment.
Noted. I had not considered mutability of the previous implementation. Consider this change to immutability a happy side-effect. At least on the master PR, this appears to have introduced no issues. Perhaps this is cause for some of the test failures I seem to have caused in the branch-2 backport. Let me investigate.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| * Construct {@link Span} instances involving data tables. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public class TableSpanBuilder implements Supplier<Span> { |
There was a problem hiding this comment.
An earlier version of this code made use of inheritance for these builders and the template gymnastics required were awful. Following Duo's suggestion, I undid that implementation in favor of this mix-in style approach with reused utility methods. Lots of code that was DRY in the inheritance version becomes repeated in this mix-in style. For me, I think this repeated code is worth the hassle, because it'll be easier to maintain than the complex hierarchy with esoteric template definitions.
All that said, if you see specific segments that could be extracted into utility methods, I'm happy to make the improvements.
| * All {@link Span}s involving {@code conn} should include these attributes. | ||
| * @see #buildConnectionAttributesMatcher(AsyncConnectionImpl) | ||
| */ | ||
| public static Matcher<SpanData> buildConnectionAttributesMatcher(ConnectionImplementation conn) { |
There was a problem hiding this comment.
Yes, Both ConnectionImplementation and AsyncConnectionImpl became public classes in order to facilitate their introspection for tracing. Not for this test class, but for their use in the Builders. They remain IA.Private, so I'm not concerned about leaking to user applications. I'm not familiar with the conversation that lead to them being package private; do you have context or specific concerns that can enlighten me?
Add support for `db.system`, `db.connection_string`, `db.user`. Signed-off-by: Duo Zhang <zhangduo@apache.org>
e85eba6 to
b28307bCompare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Josh Elser <josh.elser@gmail.com>
Apache-HBase
commented
Jan 12, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jan 12, 2022
🎊 +1 overall
This message was automatically generated. |
Addressing additional comments raised in branch-2 backport PR apache#4014
Apache-HBase
commented
Jan 12, 2022
🎊 +1 overall
This message was automatically generated. |
Add support for `db.system`, `db.connection_string`, `db.user`. Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Huaxiang Sun <huaxiangsun@apache.org> Co-authored-by: Josh Elser <josh.elser@gmail.com>
Add support for `db.system`, `db.connection_string`, `db.user`. Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Huaxiang Sun <huaxiangsun@apache.org> Co-authored-by: Josh Elser <josh.elser@gmail.com>
Addressing additional comments raised in branch-2 backport PR #4014
Addressing additional comments raised in branch-2 backport PR apache#4014
Addressing additional comments raised in branch-2 backport PR apache#4014
Addressing additional comments raised in branch-2 backport PR #4014
Addressing additional comments raised in branch-2 backport PR #4014
Add support for
db.system,db.connection_string,db.user.This is a non-trivial backport of #3952 to branch-2. It's non-trivial because it implements these tracing features on the non-async implementation of HRegionLocator. Please take a look at those changed with a critical reviewer's eye. A diff of the diffs might be useful for reviewers... I don't now if GitHub can produce such a thing.