Zen2: Add node id to log output of CoordinatorTests #33929
Conversation
ywelsch
added
>non-issue
v7.0.0
:Distributed/Cluster Coordination
|
Pinging @elastic/es-distributed |
| return runnable -> runnableConsumer.accept(wrap(runnable, node)); | ||
| } | ||
|
|
||
| private static Function<Runnable, Runnable> wrapper(DiscoveryNode node) { |
There was a problem hiding this comment.
I think I'd inline this, it's only used in one place.
There was a problem hiding this comment.
Very neat. I asked for two minor changes. Also can you think of a way to get the node name into the same place in these DeterministicTaskQueue messages:
running task n of m
advanceTime: no longer deferred
I see that it's there at the end:
... (wrapped for {node0}{hI1aFgAAQACTyUPC_____w}{0.0.0.0}{0.0.0.0:1})
but just wondered if it was possible to get it to the start as well.
| @@ -364,4 +367,28 @@ private static void processMessageReceived(TransportRequest request, RequestHand | |||
| TransportChannel transportChannel) throws Exception { | |||
| requestHandler.processMessageReceived(request, transportChannel); | |||
| } | |||
|
|
|||
| private static Consumer<Runnable> wrap(Consumer<Runnable> runnableConsumer, DiscoveryNode node) { | |||
There was a problem hiding this comment.
I think I'd inline this, it's only used in one place.
| return runnable -> wrap(runnable, node); | ||
| } | ||
|
|
||
| private static Runnable wrap(Runnable runnable, DiscoveryNode node) { |
There was a problem hiding this comment.
Could this be onNode(DiscoveryNode node, Runnable runnable) - makes it a bit more readable at its call sites?
The only possibility I see for doing this would be to extend the notion of task on DeterministicTaskQueue. I'm not sure we should do that though if we can get away with something simpler. The DeterministicTaskQueue is more of a global object (rather than a per-node level object). Maybe it's sufficient to change |
| @@ -379,7 +297,7 @@ public void run() { | |||
|
|
|||
| @Override | |||
| public String toString() { | |||
| return runnable.toString() + " (wrapped for " + node + ")"; | |||
| return node.getId() + ": " + runnable.toString(); | |||
There was a problem hiding this comment.
I think that having the UUID of the disco node will be useful here when we start rebooting nodes.
There was a problem hiding this comment.
I've added the ephemeral id
| private static void processMessageReceived(TransportRequest request, RequestHandlerRegistry requestHandler, | ||
| TransportChannel transportChannel) throws Exception { | ||
| requestHandler.processMessageReceived(request, transportChannel); | ||
| private static Runnable onNode(Runnable runnable, DiscoveryNode node) { |
There was a problem hiding this comment.
I meant to put the DiscoveryNode thing first, otherwise it ends up a long way from the function call if the Runnable is an anonymous class.
There was a problem hiding this comment.
ok. With the recent changes, there were no longer any anonymous classes. I've still changed the order
ywelsch
changed the title
|
I've run this locally on my CI |
With recent changes to the logging framework, the node name can no longer be injected into the logging output using the
node.namesetting, which means that for the CoordinatorTests (which are simulating a cluster in a fully deterministic fashion using a single thread), as all the different nodes are running under the same test thread, we are not able to distinguish which log lines are coming from which node. This PR readds logging for node ids in the CoordinatorTests, making two very small changes to DeterministicTaskQueue and TestThreadInfoPatternConverter.