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: added topic-level deduplication mechanism #3385

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - feat: added topic-level deduplication mechanism #3385

wants to merge 2 commits into from

Conversation

galibey
Copy link
Contributor

@galibey galibey commented Jul 11, 2023

Upd.: Split into two parts: first

Added topic-level deduplication mechanism.
Key impacted components:

  • Topic Config
  • Fluvio CLI (topic creation, topic list, topic describe)
  • SPU (replica and producer handler)
  • SC
  • K8s CRDs (topic and partition)

The feature requires for Dedup SmartModule filter to be provided. It is coming, but the one from the examples can be used to try it out:

cd smartmodule/examples/filter_hashset
smdk build
smdk load

then create a file topic.yaml:

version: 0.1.0
meta:
  name: dedup-topic
deduplication:
  bounds:
    count: 10
  filter:
    transform:
      uses: infinyon/fluvio-smartmodule-filter-hashset@0.1.0

and

fluvio topic create -c topic.yaml

with such configuration the window will be last 10 records.

Closes infinyon/roadmap#114

@galibey galibey requested review from digikata and sehz July 11, 2023 11:43
@galibey galibey self-assigned this Jul 11, 2023
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.

first pass

} else {
error!("follower replica {} didn't exists!", old_replica.id);
}
outputs
}

pub async fn apply_replica_update(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is these formatting changes only? If so, would prefer to move out of this PR since it's not related to this feature

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 not formatting. This is propagating errors up from replica initialization.

use std::{
ops::{Deref},
};
use std::ops::Deref;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as previously. let's move formatting changes out of this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I modify a file where there are warnings from the rust-analyzer, I fix them. In that way, such warnings will be gradually fixed. Nobody ever will fix those warnings separately unless all tasks on the project are gone.

@sehz
Copy link
Contributor

sehz commented Jul 11, 2023

This is a rather large PR. Can this break it into smaller chunks? Suggest

  1. CRD, API, CLI related changes (all configuration related)
  2. SC/SPU changes

@galibey
Copy link
Contributor Author

galibey commented Jul 12, 2023

This is a rather large PR. Can this break it into smaller chunks? Suggest

1. CRD, API, CLI related changes (all configuration related)

2. SC/SPU changes

Ok, this is big, indeed. I am going to split it into parts.

@galibey
Copy link
Contributor Author

galibey commented Jul 12, 2023

First part #3392

bors bot pushed a commit that referenced this pull request Jul 12, 2023
This is the first part of [Dedup Impl PR](#3385) split for a more convenient review process.
@galibey galibey marked this pull request as ready for review July 12, 2023 19:06
@galibey
Copy link
Contributor Author

galibey commented Jul 12, 2023

This PR only contains SPU/SC changes

@galibey galibey requested a review from sehz July 12, 2023 19:07
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. Great work interleaving Smartmodule in existing replica lifecycle

@galibey
Copy link
Contributor Author

galibey commented Jul 14, 2023

bors r+

bors bot pushed a commit that referenced this pull request Jul 14, 2023
Upd.: Split into two parts: [first](#3392)

Added topic-level deduplication mechanism. 
Key impacted components:
 - Topic Config
 - Fluvio CLI (topic creation, topic list, topic describe)
 - SPU (replica and producer handler)
 - SC
 - K8s CRDs (topic and partition)

The feature requires for Dedup SmartModule filter to be provided. It is coming, but the one from the examples can be used to try it out:
```bash
cd smartmodule/examples/filter_hashset
smdk build
smdk load
```
then create a file `topic.yaml`:
```yaml
version: 0.1.0
meta:
  name: dedup-topic
deduplication:
  bounds:
    count: 10
  filter:
    transform:
      uses: infinyon/fluvio-smartmodule-filter-hashset@0.1.0
```
and 
```bash
fluvio topic create -c topic.yaml
```
with such configuration the window will be last 10 records.

Closes infinyon/roadmap#114
@bors
Copy link

bors bot commented Jul 14, 2023

Canceled.

@galibey
Copy link
Contributor Author

galibey commented Jul 14, 2023

bors r+

bors bot pushed a commit that referenced this pull request Jul 14, 2023
Upd.: Split into two parts: [first](#3392)

Added topic-level deduplication mechanism. 
Key impacted components:
 - Topic Config
 - Fluvio CLI (topic creation, topic list, topic describe)
 - SPU (replica and producer handler)
 - SC
 - K8s CRDs (topic and partition)

The feature requires for Dedup SmartModule filter to be provided. It is coming, but the one from the examples can be used to try it out:
```bash
cd smartmodule/examples/filter_hashset
smdk build
smdk load
```
then create a file `topic.yaml`:
```yaml
version: 0.1.0
meta:
  name: dedup-topic
deduplication:
  bounds:
    count: 10
  filter:
    transform:
      uses: infinyon/fluvio-smartmodule-filter-hashset@0.1.0
```
and 
```bash
fluvio topic create -c topic.yaml
```
with such configuration the window will be last 10 records.

Closes infinyon/roadmap#114
@bors
Copy link

bors bot commented Jul 14, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title feat: added topic-level deduplication mechanism [Merged by Bors] - feat: added topic-level deduplication mechanism Jul 14, 2023
@bors bors bot closed this Jul 14, 2023
@galibey galibey deleted the feat/dedup-impl-spu-and-sc branch July 14, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants