Savonitar commented on code in PR #28857:
URL: https://github.com/apache/flink/pull/28857#discussion_r3719626895
##########
flink-runtime/src/main/java/org/apache/flink/runtime/source/coordinator/SourceCoordinatorProvider.java:
##########
@@ -75,7 +76,7 @@ public SourceCoordinatorProvider(
@Override
public OperatorCoordinator getCoordinator(OperatorCoordinator.Context
context) {
- final String coordinatorThreadName = "SourceCoordinator-" +
operatorName;
+ final String coordinatorThreadName =
createCoordinatorThreadName(context);
Review Comment:
I went through every consumer of the name:
1. CoordinatorExecutorThreadFactory.isCurrentThreadCoordinatorThread()
compares thread identity, not the name, so the checkState guards in
SourceCoordinatorContext are unaffected.
2. getCoordinatorThreadName() is used in only two places: deriving the
worker pool name, and one LOG.error message.
3. Repo-wide, "SourceCoordinator-" appears three times: once building the
name in the provider, and twice in its own test, both startsWith. There is no
exact-match assertion anywhere.
4. No metrics or logging configuration keys off thread names.
The SourceCoordinator-<operatorName> prefix is preserved, so prefix matching
keeps working for profilers and jstack greps, only an external full-string
matcher could break. Coordinator thread names already contained arbitrary
operator names, including spaces for SQL operators, so spaces and parentheses
are not a new class of problem for parsers.
One note: SourceCoordinatorContext derives the worker pool name as
getCoordinatorThreadName() + "-worker", so worker threads now read
"SourceCoordinator-<op> (job: … )-worker-thread-N" , the job suffix ends in the
middle instead of at the end. Cosmetic, and nothing parses it. I have plans to
improve that, but it will require more changes, bloat the PR, meanwhile it
looks like there is no impact.
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
