jyothsnakonisa commented on code in PR #178:
URL:
https://github.com/apache/cassandra-analytics/pull/178#discussion_r2898481383
##########
cassandra-analytics-cdc-sidecar/src/main/java/org/apache/cassandra/cdc/sidecar/SidecarStatePersister.java:
##########
Review Comment:
CdcPublisher.stop() method eventually call this stop method in state
persister, CdcPublisher.stop() gets called in the following scenarios.
1. TokenRanges changes (so that sidecar starts consumers for new token
ranges)
2. Cdc & kafka config changes.
In both the scenarios, a subsequent start is called after stop.
Now the question is, if flush fails, do we not allow restart of iterators to
handle above changes? If we don't allow restart, it will remain in the broken
state forever.
With the reversed ordering, if flush() throws and cancelTimer() would never
be called, leaving the periodic timer still firing and timerId would not be
reset, blocking any subsequent start() calls. So I think we should keep this
order to be able to restart
##########
cassandra-analytics-cdc/src/main/java/org/apache/cassandra/cdc/Cdc.java:
##########
@@ -442,5 +454,6 @@ protected void refreshSchema()
public void close()
{
this.stop();
+ statePersister.stop();
Review Comment:
Yes you are right statePersister.close() should be called when closing CDC,
If you closely notice there is another method which does that. With that said,
having public stop method in Cdc is confusing. I will make stop method in CDC
private, so that only close() method is available which does both CDC.stop()
and statePersister.stop()
```
public void close()
{
this.stop();
statePersister.stop();
}
```
##########
cassandra-analytics-cdc/src/main/java/org/apache/cassandra/cdc/Cdc.java:
##########
Review Comment:
I think the intention behind silently logging and continuing in case of
failures is not to block retrying. However I agree on your point that for
irrecoverable errors may be we should stop()
EX: if there is an out of memory exception during run method, do we stop
consumers completely? what if the memory usage goes down, how does the sidecar
start consumers again in that case?
We have to think about those scenarios before changing the behavior to fail
on irrecoverable errors.
--
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]
