xumingming commented on PR #3672:
URL: https://github.com/apache/celeborn/pull/3672#issuecomment-4494105372
@SteNicholas Answers to Claude's comments:
> - ⚠️ finally { if (needCleanupPusher) cleanupPusher(); } still runs when
the assertion throws.
TaskKilledException isn't an InterruptedException, so it bypasses the
catch; needCleanupPusher is
still true, so cleanupPusher() executes after the pusher already
terminated. Two things to
confirm: (1) cleanupPusher() is safe/idempotent post-termination, and (2)
if it throws
IOException, that exception will supersede the TaskKilledException (Java
finally semantics),
masking the real kill reason. Worth a comment or guarding.
**This is actually a good catch.** I have moved the
assertIteratorFullyConsumed call site, so when we throw the
TaskKilledException, the cleanup has not been done yet -- be consistent as
original design.
BTW: Currently the needCleanupPusher is very tricky and fragile, I'd like a
add another PR to optimize it.
> - ⚠️ Generics: scala.collection.Iterator<?> combinedIterator =
dep.aggregator().get().combineValuesByKey(...) then
write0(combinedIterator). The original passed
the expression inline (type inferred). Confirm write0 accepts the wildcard
without an
unchecked/raw warning regression.
An earlier review comment asked me to pass type parameter to the iterator.
Now it is asking them it might not be a good idea. I'm ok with either.
> Test coverage — too shallow for the risk
Drive a real writer with a partially-consumed iterator — is harder to
construct realistically than it sounds. In normal flow, doWrite() consumes the
iterator to exhaustion (while (records.hasNext())). For iteratorHasNext to be
true after doWrite(), you'd need to simulate a framework-level anomaly:
interrupt mid-loop, iterator returning spurious elements, or concurrent
modification. That test ends up testing the mock more than the code.
--
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]
