Skip to content

Conversation

@aymkhalil
Copy link
Contributor

Limit the number of inflight pulsar mutations to 1000 (hard coded for now) inspired by how the agent doesn't (independent from the blocking send capabilities in the pulsar client)

@aymkhalil aymkhalil requested a review from eolivelli March 2, 2023 22:54
@aymkhalil aymkhalil marked this pull request as ready for review March 2, 2023 23:14
// TODO: Disable the e2e latency metric if the records are emitted from cdc back-filling CLI
final long tsMicro = Instant.now().toEpochMilli() * 1000;
futures.add(mutationSender.sendMutationAsync(createMutation(pkValues.toArray(), this.exportedTable.getCassandraTable(), tsMicro)));
sendMutationAsync(createMutation(pkValues.toArray(), this.exportedTable.getCassandraTable(), tsMicro));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are not handling failures

In case of failures we must report the error and retry

One proposal:

  • keep accumulating a list of Futures
  • every 1000 futures, wait for the results (then clear the list), fail the procedure in case there is one failure

Copy link
Contributor Author

@aymkhalil aymkhalil Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error handling is implemented here: https://github.com/datastax/cdc-apache-cassandra/pull/118/files#diff-19495dbde526d4197418c17b952a3d8eb743402e9e8c7c01aafb05d503e28610R189

It logs, and the futures keep progressing. In the end, it reports the number of succeeded, failed, skipped.
@eolivelli does that address your comment at least for V0?

@aymkhalil aymkhalil requested a review from eolivelli March 6, 2023 21:19
@aymkhalil
Copy link
Contributor Author

The latest patch:

  1. Implements a fail-fast strategy
  2. Rebases on top of master branch to trigger the new CI tests

@eolivelli PTAL

Copy link
Collaborator

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@aymkhalil aymkhalil merged commit 1cdc9b4 into master Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants