SEZ9 commented on PR #12081:
URL: https://github.com/apache/seatunnel/pull/12081#issuecomment-6050688554
@Rangsh thanks for the recap, and for confirming (a) and leaving
`SplitClusterFaultToleranceIT#testStreamJobRestoreInAllNodeDown` untouched —
agreed on both.
A few notes on the items as posted; I'll confirm each against the diff
itself:
- 1a: Javadoc saying the untimed `get()` blocks indefinitely documents the
behavior but doesn't remove the hang risk the original point was about. No
production caller using it lowers the practical exposure, but I'd still prefer
either a bounded wait or an explicit in-code justification for leaving it
unbounded.
- 1b / 2 / 4: The method-level `TimeoutException` javadoc on the timed
overload, `executeResponse()` wrapped in its own `try/catch`, and the Mockito
test-scope dependency alongside the rename in `26ce5cb26` sound like what I
asked for. I'll confirm once I've looked at the diff.
- 3: Fail-closed via a sticky flag rather than reusing the writer is a
reasonable direction; I'll check the implementation. Keeping the
`Error`-escaping-`writer.write()` case out of scope and tracked in the
follow-up is fine.
The comment looks truncated again partway through item 4 ("no pom chang…"),
so two earlier points are still unaccounted for:
1. `batchQueryExecuteFailsStatus` waiting the full
`writDataTimeoutMilliseconds` per entry sequentially, so a stuck WAL worker can
stall `storeAll`/`deleteAll` for N × timeout rather than ~N × 1s — did this
change, or is it intentionally kept?
2. Every timed-out wait in `queryExecuteStatus` logging a full
`TimeoutException` stack trace at ERROR where the old code returned `false`
silently — kept as-is, downgraded, or trimmed to message-only?
Could you repost just the tail of the recap (anything after item 4) as a
fresh, shorter comment so it doesn't get cut off? Once I can see those two and
have gone through the diff, I don't expect anything else outstanding here.
<!-- streview-comment:1581 -->
--
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]
