PartitionId in the logging context #7885
Conversation
We have in the RaftContext only the name, which contains the raft group and partition id. Sometimes we just want to have the partition id as an int this introduces this property.
This context map is used on execution to set the logging context. Per default it just contains the actor name. Later sub classes should fill it with more context. It should make it easier on debugging and improve observability.
In order to not recreate the map object all the time, we create a map only once on the first call and return that afterwards.
There was a problem hiding this comment.
👍
Code looks fine, just some questions.
💭 I'm not sure this is the best approach in the long term, but it should work well enough until we have some form of actor hierarchy (whether with Akka or in our scheduler). See for example this Akka tutorial. Propagating your context is one of the advantages of having a hierarchy. Anyway, just a thought.
| import java.util.concurrent.TimeUnit; | ||
| import java.util.function.BiConsumer; | ||
| import java.util.function.Consumer; | ||
|
|
||
| public abstract class Actor implements CloseableSilently, AsyncClosable, ConcurrencyControl { | ||
|
|
||
| public static final String ACTOR_PROP_NAME = "actor-name"; |
atomix/cluster/src/main/java/io/atomix/raft/cluster/impl/RaftClusterContext.java
Outdated
Show resolved
Hide resolved
Add new method which should be overwritten by subclasses in order to add more information on which context the actor is currently running. This method is only on the first time, when the context is not yet filled.
Returning modifiable map on this method makes it easier to implement sub classes. #getContext will return an umodifiable map
|
@npepinpe please have another look |
|
@npepinpe ping |
There was a problem hiding this comment.
👍
I'm not sure why it re-ordered some things (e.g. in ZeebePartition, AsyncSnapshotDirector), can you verify before merging that it's now correctly ordered according to the style guide?
💭 I'm wondering if it might not be interesting to pass in context information when spawning an actor/submitting it to the scheduler? That way you could propagate context information.
|
Bors r+ |
|
I'm pretty sure since otherwise the other 20 files would have been reordered. Regarding the other thing I think we can do it if we need it |
7885: PartitionId in the logging context r=Zelldon a=Zelldon ## Description * Add the partition id explicitly to the raft context * Set the partition id to the MDC on the first raft thread execution * Add a context map to the actor which is used for the MDC on exection @npepinpe please take a look at this and let me know whether this approach makes sense to you. If this is the case I will change sub classes of the Actor class to add there partition id to the context. <!-- Please explain the changes you made here. --> ## Related issues <!-- Which issues are closed by this PR or are related --> closes #7859 Co-authored-by: Christopher Zell <zelldon91@googlemail.com>
|
Build failed: |
|
Bors r+ |
|
Build succeeded: |
ghost
merged commit 
|
Successfully created backport PR #7910 for |
Description
@npepinpe please take a look at this and let me know whether this approach makes sense to you. If this is the case I will change sub classes of the Actor class to add there partition id to the context.
Related issues
closes #7859
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/0.25) to the PR, in case that fails you need to create backports manually.Testing:
Documentation: