smengcl commented on PR #11146:
URL: https://github.com/apache/ozone/pull/11146#issuecomment-5825822711
Reviewed the current head. The four open comments from @rich7420 all hold,
and so does the global-counter point (it reads as outdated but the underlying
issue is still there). Below are only the issues **not** already raised in an
existing comment. I have a patch for these that I can push to the branch if you
like.
### 1. `waitForReplicaCount(scmContainerId, 3, cluster)` is satisfied on its
first poll
`DeadNodeHandler` removes a datanode's replicas only once SCM declares the
node DEAD (`ozone.scm.dead.node.interval` = 6s here). The wait runs
milliseconds after `shutdownHddsDatanode()` returns, while SCM still has all
three replicas registered, so the first poll succeeds and the method returns
immediately.
The container is never observed under-replicated and re-replication never
runs: this half of the test would pass identically with `ReplicationManager`
disabled. Waiting for `DEAD` first, then for a three-replica set that excludes
the stopped node, makes it deterministic and non-vacuous.
### 2. `replicas.iterator().next()` can throw `NoSuchElementException`
`waitForContainerClose()` polls datanode-side state only; SCM learns about
replicas asynchronously via ICR. Immediately after it returns,
`getContainerReplicas()` may hold fewer than three replicas, and in the worst
case none, so `iterator().next()` throws instead of failing with a readable
message. Settling on the replica count first fixes both this and (1).
### 3. `restoreBadVolume(vol0)` is not in a `finally`
`simulateBadVolume()` does `setWritable(false)`. If any assertion between
the failure injection and the last line fails, the restore never runs: the
decommission test then executes against a datanode with a permanently read-only
volume, and `cluster.shutdown()` cannot delete its base dir (the exception is
swallowed), leaking a read-only temp tree.
Worth noting separately that `restoreBadVolume` only flips the file
permission. The volume stays in `getFailedVolumesList()` and its pool is not
recreated, so it does not undo as much as the name suggests.
### 4. The decommission test's wait budget exceeds the 5m default JUnit
timeout
The root `pom.xml` sets `junit.jupiter.execution.timeout.default = 5m` for
every module, and `integration-test` overrides only `argLine`.
`testDecommissionWithPerVolumePools` chains `waitForKeyContainer` x2 (60s each)
+ `waitForDnToReachOpState` (30s) + `waitForContainerReplicas` x2 (60s) +
`waitForDnToReachHealthState` (30s) + `waitForContainerReplicas` x2 (60s) =
420s worst case, plus 40 key writes.
A slow CI agent aborts with a bare JUnit timeout naming no predicate,
instead of the `waitFor` failure that would say which replica count never
settled. `TestDecommissionAndMaintenance` uses `200, 30000` for the same waits.
### 5. `bucket` outlives the `OzoneClient` it came from
`setUp()` assigns the `bucket` field inside `try (OzoneClient client =
cluster.newClient())`, so the client is closed before any test runs, while all
three tests keep writing and reading keys through that bucket handle.
To be precise about severity: this does work today, because
`RpcClient.close()` ends at `RPC.stopProxy`, and `ClientCache` refcounts the
shared `ipc.Client` that the rest of the mini-cluster keeps alive, so the proxy
stays usable. It is working by accident rather than by contract, and
`XceiverClientManager.close()` has already invalidated the client cache and
unregistered its metrics by then. `TestDecommissionAndMaintenance` holds
`client` as a field and closes it in teardown; doing the same here costs three
lines.
### 6. `UniformDatanodesFactory` silently resets every replication config key
This is the reason `newCluster()` needs its read/copy/write dance at all.
`configureDatanodePorts()` does:
```java
conf.setFromObject(new
ReplicationServer.ReplicationConfig().setPort(getFreePort()));
```
`setFromObject` writes every `@Config` field of a default-constructed
object, so it resets `per.volume.enabled` to false, `per.volume.streams.limit`
to 2, `streams.limit` to 10, `queue.limit` to 4096 and
`outofservice.limit.factor` to 2.0 for **every** test using this factory.
The wrapper here restores only two of those six, so any other replication
setting a future test puts in the cluster conf is still dropped, and the next
person to hit this has to reinvent the same workaround. Setting just the port
key in the factory would remove the need for the wrapper entirely. I have left
this out of my patch because it is a change to shared test infrastructure
rather than to this PR's test, but it seems worth a follow-up.
### 7. `@Execution(SAME_THREAD)` and `@ResourceLock("MiniOzoneCluster")` are
no-ops
`pom.xml` already sets `parallel.mode.default = same_thread` and
`parallel.mode.classes.default = same_thread` for all modules, so `SAME_THREAD`
is the default. `@ResourceLock("MiniOzoneCluster")` is the only occurrence of
that key in `integration-test`, so it can never contend with anything. Both
read as a concurrency contract the build does not have.
### 8. Smaller items
- `assertTrue(supervisor.getReplicationFailureCount() >= previousFailures +
1)` prints only "expected true" and hides both counts; same for
`assertEquals(1, volSet.getFailedVolumesList().size())` and the `hasPool`
assertions. AssertJ (`assertThat(...).isGreaterThanOrEqualTo(...)`,
`.hasSize(1)`) makes a CI failure diagnosable from the report alone, per
HDDS-9951.
- `createSharedConfig` -> `createDecommissionConfig(boolean, int)` ->
`createPerVolumeConfig(boolean, int)` is three methods for one configuration;
both parameters only ever receive `(true, 1)`, and
`ReplicationManagerConfiguration` is fetched and written back twice across two
of them.
- The bare `getContainer(cluster, target, containerId)` calls after
`queuePushAndWaitForContainer` are left over from the `assertNotNull` removal.
The wait already confirmed presence via `hasContainer`, so these can only fire
if the container vanished in the intervening microseconds.
- The file mixes the static import `UTF_8` with `StandardCharsets.UTF_8` for
the same charset.
### On what the tests assert
Expanding on @rich7420's question at `testDecommissionWithPerVolumePools`,
since it applies to the class as a whole: `assertVolumePools` and
`waitForVolumePoolState` check pool existence and size, which
`TestReplicationSupervisor#perVolumePoolSizeRespected` and its failed-volume
test already cover without a cluster. Nothing currently asserts that a push was
actually *dispatched* to a per-volume executor, so a push that ran entirely on
the global pool would satisfy every assertion here.
Two assertions would make the cluster earn its place, and both are reachable
from this package without touching production code:
- `pools.getExecutor(volumeRoot)` is package-private and returns the
`ThreadPoolExecutor`, so `getCompletedTaskCount()` before/after a push proves
the task ran on that volume's pool.
- For decommission specifically, SCM ->
`SetNodeOperationalStateCommandHandler` -> `supervisor::nodeStateUpdated` ->
`volumePools.setPoolSize(scaleOutOfServiceLimit(...))` is a path no unit test
exercises end to end. Asserting the per-volume pools resize from 1 to `ceil(1 x
2.0)` = 2 once the node reaches `DECOMMISSIONED` is per-volume behaviour that
the existing decommission test cannot catch.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
