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

Feature/init sync #11

Merged
merged 7 commits into from
Mar 8, 2023
Merged

Feature/init sync #11

merged 7 commits into from
Mar 8, 2023

Conversation

ryanjclark
Copy link

@ryanjclark ryanjclark commented Mar 3, 2023

  • Bringing upstream commit in. This upgrades amazon-kinesis-client and dynamodb-streams-kinesis-adapter and adds some CloudWatch metrics.
  • Extending connector to include init.sync.skip flag. This optional flag will skip the initial load of dynamodb and read from LATEST on the DynamoDBStream.

Copy link
Collaborator

@psheets psheets left a comment

Choose a reason for hiding this comment

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

Does the flag need to be init.sync.skip = true? could you use init.sync = false?

.define(SRC_INIT_SYNC_DELAY_CONFIG,
ConfigDef.Type.INT,
SRC_INIT_SYNC_DELAY_DEFAULT,
ConfigDef.Importance.LOW,
SRC_INIT_SYNC_DELAY_DOC,
CONNECTOR_GROUP, 2,
CONNECTOR_GROUP, 3,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is 'CONNECTOR_GROUP' ?

Copy link
Author

Choose a reason for hiding this comment

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

This class is used to display configs in the UI on something like Confluent Hub (width, grouping, importance). No real use outside of that.

HashMap<String, Object> offset = new HashMap<>();
offset.put("table_name", tableName);
offset.put("init_sync_state", "SKIPPED");
offset.put("init_sync_start", Instant.parse("2001-01-02T00:00:00.00Z").toEpochMilli());
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this require putting a sync_start time even if the init sync is skipped?

Copy link
Author

Choose a reason for hiding this comment

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

That function does require it- which shouldn't be a problem because init sync skip logic puts that field to 0. If it's coming from an offset, it means it will have that field already.

I'll change the test to be epoch 0.

@mickray
Copy link

mickray commented Mar 3, 2023

Can you add a comment to the README documenting this setting?

@Gurjit86
Copy link

Gurjit86 commented Mar 3, 2023

for larger stream - do we want to increase Stream read limit - default is 1000 records - configured under com.trustpilot.connector.dynamodb.Constants.java

@ryanjclark
Copy link
Author

ryanjclark commented Mar 3, 2023

for larger stream - do we want to increase Stream read limit - default is 1000 records - configured under com.trustpilot.connector.dynamodb.Constants.java

That's a great find. Looks like that class' default is 10,000- not 1,000. I will address throughput in the next PR.

Does the flag need to be init.sync.skip = true? could you use init.sync = false?

There's no init.sync option that's why I'm making one. Init sync is default behavior so I named the flag init.sync.skip.

Can you add a comment to the README documenting this setting?

Take a look at the README and documentation.

@mickray
Copy link

mickray commented Mar 3, 2023

Can you add a comment to the README documenting this setting?

Take a look at the README and documentation.

Whoops- missed that. Thanks for updating

@ryanjclark ryanjclark merged commit 3f4c270 into master Mar 8, 2023
@ryanjclark ryanjclark deleted the feature/init-sync branch March 8, 2023 01:06
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.

5 participants