david-streamlio commented on PR #17:
URL: https://github.com/apache/pulsar-connectors/pull/17#issuecomment-4936593186
Heads-up, @harangozop: this PR is now `CONFLICTING` — #39 ("drain flush()
iteratively to avoid StackOverflowError") merged and rewrote the same `flush()`
method in `JdbcAbstractSink`. Worth rebasing and addressing the outstanding
review point in one pass rather than two.
The review finding still stands independently of the conflict. CI caught a
real defect in the new `close()`:
```
java.lang.NullPointerException: Cannot invoke
"LinkedBlockingDeque.drainTo(java.util.Collection)" because "this.incomingList"
is null
at
org.apache.pulsar.io.jdbc.JdbcAbstractSink.close(JdbcAbstractSink.java:182)
at
org.apache.pulsar.io.jdbc.PostgresJdbcArrayIntegrationTest.tearDown(...)
```
`incomingList` is assigned only in `open()`, so `close()` NPEs whenever the
sink is closed without a successful open — which Pulsar IO does on startup
failure, and which tests do in `tearDown`. Guarding the drain fixes it:
```java
if (incomingList != null) {
List<Record<T>> remaining = new ArrayList<>();
incomingList.drainTo(remaining);
remaining.forEach(Record::fail);
}
```
One thing worth knowing when you rebase: #39 replaced the recursive
`flush()` with an iterative `do { ... } while (needAnotherRound)` loop, which
overlaps with the recursion removal in this PR. Some of what this PR does there
may now be redundant — the back-pressure change (`LinkedBlockingDeque` +
blocking `offer(timeout)`) is the part that remains distinctly valuable, since
#39 did not address the nack/redeliver storm from #16.
Happy to help with the rebase if useful.
--
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]
