baiyangtx commented on PR #4282:
URL: https://github.com/apache/amoro/pull/4282#issuecomment-5033599633
Thanks for the fix! The direction is correct: the previous `Map<String,
ActionCoordinatorScheduler>` keyed by `action().getName()` silently overwrites
entries when multiple coordinators share the same action name but target
different `TableFormat`s (exactly the mixed-iceberg case in
#4281). Switching to `Map<String, List<ActionCoordinatorScheduler>>` +
dispatching by `TableFormat` addresses the root cause.
A few things to address before merge.
## 1. Missing unit tests (blocker)
The PR description checks "Add some test cases … including negative and
positive cases" but the diff contains no test changes. Please add at least one
test that pins down the regression:
- Register two `MockActionCoordinator`s with the **same**
`action().getName()` but different `formatSupported` (e.g. one accepts only
`PAIMON`, one only `MIXED_ICEBERG`).
- Fire `handleTableAdded` / `handleStatusChanged` with a `TableRuntime` of
each format.
- Assert each event lands **only** on the matching scheduler.
Without this, the bug can silently regress again if `actionCoordinators`
is ever re-keyed or the dispatch helpers are refactored.
## 2. `(action, format)` uniqueness is not enforced anywhere
`AbstractPluginManager` only enforces uniqueness of `plugin.name()` (see
`foundedPlugins` at `AbstractPluginManager.java:344-358` and `pluginConfigs` at
`AbstractPluginManager.java:107-115`). Two `ActionCoordinator`s can
legitimately share the same `action().getName()` and overlapping
`formatSupported(...)` as long as their `plugin.name()` differ. In that
case `findScheduler` returns the first match via `findFirst` and the second
coordinator is silently ignored — the same class of bug as before, just harder
to see (both schedulers exist in the map).
Please validate `(action().getName(), format)` uniqueness at registration
time (in `initialize` and `installActionCoordinator`) and fail fast, or at
minimum `LOG.warn` with both coordinators' plugin names so operators can spot
the collision on startup.
## 3. `recoverProcesses` now silently drops records when no
format-matching scheduler exists
```java
if (tableRuntime != null) {
ActionCoordinatorScheduler scheduler =
findScheduler(processMeta.getProcessType(),
tableRuntime.getFormat());
if (scheduler != null) {
recoverProcess(tableRuntime, scheduler, processMeta);
}
}
```
Previously any scheduler registered under that action name would recover
the process; now the format also has to match. This is desirable, but if a
coordinator's format support changes across releases, historical process rows
will be skipped with no signal. Please add a LOG.warn in the
scheduler == null branch including processId, processType, and
tableRuntime.getFormat() to keep this diagnosable.
ProcessService.java:193-204
## 4. executeOrTraceProcess.onProcessFinished — silent behavior change
when tableRuntime == null
```java
ActionCoordinatorScheduler scheduler = null;
if (process.getTableRuntime() != null) {
scheduler =
findScheduler(store.getAction().getName(),
process.getTableRuntime().getFormat());
}
```
Before, a null tableRuntime could still resolve a scheduler by action name
and trigger retry. After this change we never retry in that branch. That's
probably the intended behavior (we can't classify format without a runtime),
but it deserves a LOG.info or LOG.debug so we don't wonder why
a FAILED process stopped retrying.
ProcessService.java:292-316
## 5. Inner ArrayList is not concurrent-safe
actionCoordinators is a ConcurrentHashMap, but each value is a plain new
ArrayList<>(). Today writes happen in initialize() (single-threaded startup)
and @VisibleForTesting installActionCoordinator, while reads happen from event
handlers on runtime threads. That's safe only if init
strictly happens-before any handler dispatch. If installActionCoordinator
(or a future hot-install path) ever runs after startup, the current structure
will race.
Either:
- switch the inner list to CopyOnWriteArrayList, or
- add a Javadoc/inline comment stating "no writes after initialize()
returns" and rely on the existing lifecycle guarantee.
ProcessService.java:63-64, 166-169, 514-518
## 6. @VisibleForTesting return type change ripples to callers
getActionCoordinators() changed from Map<String,
ActionCoordinatorScheduler> to Map<String, List<ActionCoordinatorScheduler>>.
Please double-check every existing test that consumes this (e.g. under
amoro-ams/src/test/java/org/apache/amoro/server/process/... and
amoro-paimon-internal
integration tests) still compiles and asserts sensibly — a local mvn -pl
amoro-ams,amoro-paimon-internal test-compile should be enough.
———
Minimum bar to merge
1. Add the regression test from (1).
2. Enforce (action, format) uniqueness at registration (2).
3. Add the missing LOG.warn in recoverProcesses (3).
(4), (5), (6) are nice-to-haves.
--
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]
