jojochuang commented on PR #8698: URL: https://github.com/apache/hadoop/pull/8698#issuecomment-5721574503
Thanks for the follow-up on HDFS-17897 / HDFS-17899 — this closes the remaining datanode outbound paths (`writeBlock` mirror, `replaceBlock` proxy, `StripedBlockWriter` EC setup) with the same clear-key-and-retry-once pattern. I'll **+1 and merge** this. No blockers from my side. If you want to iterate later, here are improvements worth considering (from review in Cursor — all optional follow-ups): 1. **Checkstyle / method length** — CI flagged `replaceBlock` at 158 lines (limit 150). Consider extracting the proxy (and mirror) connect + SASL retry loop into a small helper to shrink the method and reduce duplication. 2. **DRY for retry helper** — `prepareRetryAfterInvalidEncryptionKey` is duplicated in `DataXceiver` and `StripedBlockWriter`. A shared package-private helper (similar spirit to #8383, but one place for DataNode-side transfer) would be easier to maintain. 3. **Stronger EC test** — `TestStripedBlockWriterEncryptionKey` currently only exercises the static retry helper. A follow-up test that mocks `getSaslClient().socketSend` and asserts reconnect + `clearDataEncryptionKey()` through `StripedBlockWriter.init()` would match the depth of `TestDataXceiverEncryptionKey`. 4. **`testReplaceBlockRetriesInvalidEncryptionKeyToProxy`** — the broad `catch (Exception ignored)` works for setup-only coverage but is brittle; narrowing to expected post-retry failures (e.g. EOF/IO when reading the fake proxy response) would make regressions easier to spot. 5. **Logging parity** — `writeBlock` / `replaceBlock` log at INFO on retry; `StripedBlockWriter` retries silently. A consistent INFO line on EC retry would help ops/debugging. 6. **Replication mirror behavior (design note, not introduced here)** — after a second `InvalidEncryptionKeyException`, non-client replication still follows the existing “continue without mirror” path. Fine for this PR; worth a separate JIRA if we ever want stricter failure semantics on encrypted pipelines. Thanks again for pushing this through. -- 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]
