Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DBZ-2288 Events in exported snapshot no longer filtered by LSN #1697

Merged
merged 5 commits into from Jul 16, 2020

Conversation

jpechane
Copy link
Contributor

Copy link
Member

@gunnarmorling gunnarmorling left a comment

Choose a reason for hiding this comment

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

A few comments inline.

@@ -88,6 +88,7 @@
* @param dropSlotOnClose whether the replication slot should be dropped once the connection is closed
* @param statusUpdateInterval the interval at which the replication connection should periodically send status
* @param exportSnapshot whether the replication should export a snapshot when created
* @param exportSnapshot whether the connector is doing snapshot
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param exportSnapshot whether the connector is doing snapshot
* @param doSnapshot whether the connector is doing snapshot

@@ -206,6 +206,14 @@ static String format(long lsn) {
*/
Builder exportSnapshotOnCreate(final boolean exportSnapshot);

/**
* Whether or not the snapshot is executed
* @param doSnapshot true if a snapshot should is going to be executed, false if otherwise
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param doSnapshot true if a snapshot should is going to be executed, false if otherwise
* @param doSnapshot true if a snapshot is going to be executed, false if otherwise

return true;
}
else {
LOGGER.trace("Streaming requested from LSN {} but received LSN {} that is same or smaller so skipping the message", startLsn, lastReceivedLsn);
Copy link
Member

Choose a reason for hiding this comment

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

But are you skipping it in this case?

* Whether or not the snapshot is executed
* @param doSnapshot true if a snapshot should is going to be executed, false if otherwise
* @return this instance
* @see #DEFAULT_EXPORT_SNAPSHOT
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't apply here?

@jpechane
Copy link
Contributor Author

@gunnarmorling Comments addressed

@gunnarmorling
Copy link
Member

@jpechane, what's still unclear to me: does this only fix the case when we create the slot, or also when it already exists (both with exported only, of course)?

@jpechane
Copy link
Contributor Author

@gunnarmorling See https://issues.redhat.com/browse/DBZ-2288?focusedCommentId=14242107&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14242107
Also the test verifies the situation when the slot already exists. We would not be able to tests the situation when we create the slot with automated tests unless we provide some hook into the connector code

Copy link
Member

@gunnarmorling gunnarmorling left a comment

Choose a reason for hiding this comment

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

@jpechane, two more small comments inline. Feel free to merge yourself afterwards.

pgConnection.commit();
}
catch (Exception e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

Rather rethrow it?

pgConnection.commit();
}
catch (Exception e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

As above.

@gunnarmorling
Copy link
Member

@jpechane, ah one more thing: it'd be good to put a note to the docs to prefer exported snaphots and the risk associated to not using them. Could also say that exported will be the default in a future version.

@jpechane jpechane merged commit c2a40b7 into debezium:master Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants