Convert Attributes to an interface #2134
Conversation
jkwatson
requested review from
anuraaga,
arminru,
bogdandrutu,
carlosalberto,
Oberon00,
pavolloffay,
thisthat and
tylerbenson
as code owners
Codecov Report
@@ Coverage Diff @@
## master #2134 +/- ##
============================================
- Coverage 85.02% 84.99% -0.03%
+ Complexity 2168 2159 -9
============================================
Files 248 249 +1
Lines 8318 8291 -27
Branches 924 917 -7
============================================
- Hits 7072 7047 -25
- Misses 909 910 +1
+ Partials 337 334 -3 Continue to review full report at Codecov.
|
|
|
||
| @AutoValue | ||
| @Immutable | ||
| abstract static class ArrayBackedAttributes extends Attributes { | ||
| abstract class ArrayBackedAttributes extends ImmutableKeyValuePairs<AttributeKey, Object> |
There was a problem hiding this comment.
This class is public - can you move it to a top level package private class?
There was a problem hiding this comment.
ooh. good point. yes, will do so.
| @@ -20,16 +23,25 @@ | |||
| * <p>Null keys will be silently dropped. | |||
| * | |||
| * <p>Note: The behavior of null-valued attributes is undefined, and hence strongly discouraged. | |||
| * | |||
| * <p>Implementations of this interface *must* be immutable and have well-defined value-based | |||
There was a problem hiding this comment.
By the way - must they be immutable? Actually one of the, completely unsupported but wild, use cases that came up for me for this interface is mutable attributes in resource. Docker cgroups come to mind, but I wouldn't be surprised if there are some good use cases for process-related information that is updatable at runtime.
There was a problem hiding this comment.
I'd much rather start by strongly suggesting immutability, then maybe relax it if we find it's ok for them to be mutable.
| @@ -39,47 +51,65 @@ | |||
| public AttributesBuilder toBuilder() { | |||
There was a problem hiding this comment.
in order for the javaagent to provide a single implementation that satisfies both the shaded and unshaded interfaces, we need the return value here to be an interface, so that its implementation use a covariant return type that satisfies both, similar to the TraceState.getTraceState() example in #1946 (comment)
There was a problem hiding this comment.
done. please take a look
|
@marcingrzejszczak FYI this is potentially a breaking change |
jkwatson
force-pushed
the
attributes_as_interface
branch
from
f3dfa20 to
f19b5a5
Compare
jkwatson
force-pushed
the
attributes_as_interface
branch
from
f19b5a5 to
bf41f09
Compare
No description provided.