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

Adding the context to the source connectors #388

Merged

Conversation

aindriu-aiven
Copy link
Contributor

Updating and refactoring the distribution strategies based on feedback.

@Override
public void configureDistributionStrategy(final int maxTasks) {
this.maxTasks = maxTasks;
return Math.floorMod(ctx.getStorageKey().hashCode(), maxTasks);
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be too slow a calculation but we can leave it for now. There is a much faster mod calculation in commons collections, but we would have to extract it.

@aindriu-aiven aindriu-aiven force-pushed the aindriu-aiven/add-context branch 7 times, most recently from 772c511 to e8452bf Compare January 17, 2025 14:32
@aindriu-aiven aindriu-aiven marked this pull request as ready for review January 17, 2025 14:34
@aindriu-aiven aindriu-aiven requested review from a team as code owners January 17, 2025 14:34
Copy link
Contributor

@Claudenw Claudenw left a comment

Choose a reason for hiding this comment

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

Need a test case for DistributionStrategy that shows negative hash values generate do not generate negative tasks or throw exceptions.

@@ -27,18 +28,34 @@
* sequentially by the same worker, which can be useful for maintaining order between objects. There are usually fewer
* workers than tasks, and they will be assigned the remaining tasks as work completes.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still a true statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I believe so, i reviewed it before and I do think it is a true statement as every object in the s3 case will only ever be able to be processed by the same task.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it depends upon the strategy implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded it

@aindriu-aiven aindriu-aiven force-pushed the aindriu-aiven/add-context branch from b8e2b2a to 9525803 Compare January 17, 2025 16:30
@aindriu-aiven aindriu-aiven force-pushed the aindriu-aiven/add-context branch from b226d52 to f4a470b Compare January 20, 2025 09:17
Copy link
Contributor

@Claudenw Claudenw left a comment

Choose a reason for hiding this comment

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

Changes look good.

@@ -27,18 +28,34 @@
* sequentially by the same worker, which can be useful for maintaining order between objects. There are usually fewer
* workers than tasks, and they will be assigned the remaining tasks as work completes.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it depends upon the strategy implemented.

Copy link
Contributor

@muralibasani muralibasani left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@aindriu-aiven aindriu-aiven force-pushed the aindriu-aiven/add-context branch from 5cfecf7 to aa0ca1e Compare January 20, 2025 11:59
@Claudenw Claudenw merged commit f68b0a4 into Aiven-Open:s3-source-release Jan 20, 2025
8 checks passed
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