Make RequestScopedMdc properties are inherited
#2729
Conversation
Motivation:
A user will expect the following case works:
ServiceRequestContext sctx = ...;
try (SafeCloseable ignored = sctx.push()) {
RequestScopedMdc.put(sctx, "transactionId", "1234");
ClientRequestContext cctx = ...;
try (SafeCloseable ignored2 = cctx.push()) {
assert MDC.get("transactionId").equals("1234");
}
}
.. which does not work currently, because `RequestScopedMdc` does not
look up the root context's map.
Modifications:
- Make `RequestScopedMdc.get()`, `getAll()` and `getCopyOfContextMap()`
look for the root context map.
Result:
- `RequestScopedMdc` properties are inherited from the root context.
for faster `putAll()` iteration
Codecov Report
@@ Coverage Diff @@
## master #2729 +/- ##
============================================
- Coverage 72.68% 72.62% -0.07%
+ Complexity 11638 11631 -7
============================================
Files 1025 1025
Lines 45568 45605 +37
Branches 5674 5683 +9
============================================
- Hits 33120 33119 -1
- Misses 9541 9566 +25
- Partials 2907 2920 +13 Continue to review full report at Codecov.
|
| } | ||
| ctx.setAttr(MAP, Collections.unmodifiableMap(newMap)); | ||
| } catch (Throwable t) { | ||
| Exceptions.throwUnsafely(t); |
There was a problem hiding this comment.
Isn't it better to log (maybe once?) and return delegate.getCopyOfContextMap(); in this case?
There was a problem hiding this comment.
We will never reach here, because we made sure invokeExact() works in the class initializer. If any exception is raised, it means it's a bug. Let me add some comment about this.
core/src/main/java/com/linecorp/armeria/common/logging/RequestScopedMdc.java
Outdated
Show resolved
Hide resolved
| final Object2ObjectOpenHashMap<String, String> merged; | ||
| if (requestScopedMap instanceof Object2ObjectOpenHashMap) { | ||
| // Reuse the mutable copy returned by getAll() for less memory footprint. | ||
| merged = (Object2ObjectOpenHashMap<String, String>) requestScopedMap; |
There was a problem hiding this comment.
Isn't this unmodifiable if the map is returned from here? https://github.com/line/armeria/pull/2729/files#diff-b35989b7cd6a82522044209c418a377dR193
There was a problem hiding this comment.
Yes. The map will not be an Object2ObjectOpenHashMap but be an Object2ObjectMaps.Unmodifiable or something. This code path is for the case where getAll() returns a mutable map, so we do not create two maps when merging twice.
There was a problem hiding this comment.
Ah I thought the condition was requestScopedMap instanceof Object2ObjectMap. Sorry about it. 😉
|
Thanks for reviewing. |
Motivation:
A user will expect the following case works:
ServiceRequestContext sctx = ...;
try (SafeCloseable ignored = sctx.push()) {
RequestScopedMdc.put(sctx, "transactionId", "1234");
ClientRequestContext cctx = ...;
try (SafeCloseable ignored2 = cctx.push()) {
assert MDC.get("transactionId").equals("1234");
}
}
.. which does not work currently, because `RequestScopedMdc` does not
look up the root context's map.
Modifications:
- Make `RequestScopedMdc.get()`, `getAll()` and `getCopyOfContextMap()`
look for the root context map.
- Reduced the memory footprint of overall map iteration by using
only `Object2ObjectMap`s.
- `Object2ObjectMap` uses a special variant of `Iterator` when iterating
over other `Object2ObjectMap`s.
- Reduced the memory footprint of `copyAll()` and `getCopyOfContextMap()`
by using `getPropertyMap()`.
Result:
- `RequestScopedMdc` properties are inherited from the root context.
Motivation:
A user will expect the following case works:
.. which does not work currently, because
RequestScopedMdcdoes notlook up the root context's map.
Modifications:
RequestScopedMdc.get(),getAll()andgetCopyOfContextMap()look for the root context map.
only
Object2ObjectMaps.Object2ObjectMapuses a special variant ofIteratorwhen iteratingover other
Object2ObjectMaps.copyAll()andgetCopyOfContextMap()by using
getPropertyMap().Result:
RequestScopedMdcproperties are inherited from the root context.