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

[Merged by Bors] - feat: Add Partitioner trait with SiphashRoundRobinPartitioner impl #965

Closed

Conversation

nicholastmosher
Copy link
Contributor

@nicholastmosher nicholastmosher commented Apr 16, 2021

Closes #838

  • Introduces Partitioner trait to enable new implementations in the future
  • Adds SiphashRoundRobinPartitioner as a first implementor of Partitioner
    • Uses Siphash-2-4 on records that have keys
    • Uses round-robin to assign partitions to records with no keys
  • Adds a StoreContext<TopicSpec> to MetdataStores for syncing topic metadata
  • Implements watch requests on SPU for Topic and SPG metadata

@sehz
Copy link
Contributor

sehz commented Apr 16, 2021

Can you link this to issue?

src/client/src/producer.rs Outdated Show resolved Hide resolved
.push(DefaultBatch::new(records));
topic_request.name = self.topic.clone();
topic_request.partitions.push(partition_request);
request.acks = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be hard coded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how the producer request was already being built (see the removed diff in send_records_raw. If we want to change this we should do it in a new issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Let's create issue to allow developers to configure producer and profile. We probably need producer builder or something like that.

src/client/src/producer.rs Outdated Show resolved Hide resolved
src/client/src/producer.rs Outdated Show resolved Hide resolved
src/client/src/producer.rs Show resolved Hide resolved
src/client/src/sync/controller.rs Outdated Show resolved Hide resolved
src/client/src/sync/controller.rs Outdated Show resolved Hide resolved
src/client/src/sync/controller.rs Outdated Show resolved Hide resolved
src/client/src/sync/controller.rs Show resolved Hide resolved
src/client/src/sync/mod.rs Outdated Show resolved Hide resolved
src/client/src/producer.rs Outdated Show resolved Hide resolved
@nicholastmosher
Copy link
Contributor Author

Ok, newest changes:

  • Updated Partitioner trait to look like the following:
trait Partitioner {
    fn partition(&mut self, keys: &[Option<&[u8]>]) -> Vec<i32>;
    fn update_config(&mut self, config: PartitionerConfig);
}
  • Added PartitionerConfig to hold information such as partition_count in a way that lets us expand in the future
  • Moved interior mutability out of Partitioner itself and up into the TopicProducer level

src/client/src/producer.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@sehz sehz left a comment

Choose a reason for hiding this comment

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

Good progress. See other comments regarding moving keyless api as separate issue

src/client/src/producer.rs Outdated Show resolved Hide resolved
src/client/src/producer.rs Outdated Show resolved Hide resolved
@sehz
Copy link
Contributor

sehz commented Apr 19, 2021

Also, test with replication factor = 2

@sehz
Copy link
Contributor

sehz commented Apr 20, 2021

Can you add this to changelog?

Copy link
Contributor

@sehz sehz left a comment

Choose a reason for hiding this comment

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

Making good progress. Couple of minor changes needed

examples/00-produce/src/main.rs Outdated Show resolved Hide resolved
examples/03-echo/src/main.rs Outdated Show resolved Hide resolved
src/cli/src/consumer/produce/mod.rs Outdated Show resolved Hide resolved
src/client/src/producer.rs Show resolved Hide resolved
src/client/src/producer.rs Outdated Show resolved Hide resolved
src/client/src/producer.rs Show resolved Hide resolved
tests/runner/src/tests/smoke/produce.rs Outdated Show resolved Hide resolved
tests/runner/src/tests/producer_stress.rs Outdated Show resolved Hide resolved
tests/runner/src/tests/concurrent/producer.rs Outdated Show resolved Hide resolved
src/client/src/producer.rs Show resolved Hide resolved
src/client/src/producer.rs Show resolved Hide resolved
src/client/src/producer.rs Outdated Show resolved Hide resolved
src/client/src/producer.rs Outdated Show resolved Hide resolved
src/client/src/producer.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@sehz sehz left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work!

@sehz
Copy link
Contributor

sehz commented Apr 20, 2021

bors r+

bors bot pushed a commit that referenced this pull request Apr 20, 2021
)

Closes #838 

- Introduces `Partitioner` trait to enable new implementations in the future
- Adds `SiphashRoundRobinPartitioner` as a first implementor of `Partitioner`
  - Uses Siphash-2-4 on records that have keys
  - Uses round-robin to assign partitions to records with no keys
- Adds a `StoreContext<TopicSpec>` to `MetdataStores` for syncing topic metadata
- Implements watch requests on SPU for Topic and SPG metadata
@bors bors bot changed the title feat: Add Partitioner trait with SiphashRoundRobinPartitioner impl [Merged by Bors] - feat: Add Partitioner trait with SiphashRoundRobinPartitioner impl Apr 20, 2021
@bors bors bot closed this Apr 20, 2021
@nicholastmosher nicholastmosher deleted the key-partitioning branch April 20, 2021 23:24
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.

Implement initial Key Partitioning strategy for key/value records
3 participants