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.

Sign up for GitHub

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

Jump to bottom

feat: add deduplicate parameter to create topic using cli #4386

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: add deduplicate parameter to create topic using cli #4386

wants to merge 3 commits into from
+106 −18

Conversation

fraidev
Copy link
Contributor

@fraidev fraidev commented Feb 11, 2025 •
edited
Loading

Solves #4388

@fraidev fraidev force-pushed the dedup_cli branch 2 times, most recently from b32e785 to eb5543c Compare February 11, 2025 04:40
@fraidev fraidev changed the title feat: add deduplicate to create topic cli feat:add deduplicate opt for create topic cli Feb 11, 2025
@fraidev fraidev changed the title feat:add deduplicate opt for create topic cli feat: add deduplicate opt for create topic cli Feb 11, 2025
@fraidev fraidev force-pushed the dedup_cli branch 2 times, most recently from 1cca184 to e2da55a Compare February 11, 2025 05:40
@ajhunyady
Copy link
Contributor

ajhunyady commented Feb 11, 2025 •
edited
Loading

@fraidev, we should rename deduplicate to dedup, for easier typing.

I also think we should expose 2 variants:

  1. PREFERRED: Add a convenience command that will download the fluvio/dedup-bloom-filter@0.1.0 smartmodule automatically.

    fluvio topic create dedup_topic --dedup
fluvio topic create dedup_topic --dedup --dedup-age 1h --dedup-count 5

  2. ADVANCED: Expose the same --smartmodule command that works with fluvio consume for the user to apply a user-defined smartmodules and parameters:

    fluvio hub sm download fluvio/dedup-bloom-filter@0.1.0
     fluvio topic create dedup_topic --smartmodule fluvio/dedup-bloom-filter@0.1.0
 fluvio topic create dedup_topic --smartmodule fluvio/dedup-bloom-filter@0.1.0 -e dedup-page=1h  -e dedup-count=5

@fraidev fraidev mentioned this pull request Feb 11, 2025
Open
@fraidev fraidev force-pushed the dedup_cli branch 2 times, most recently from 4a49404 to 329b92b Compare February 11, 2025 17:50
@fraidev
Copy link
Contributor Author

fraidev commented Feb 11, 2025

2. --smartmodule

@fraidev, we should rename deduplicate to dedup, for easier typing.

I also think we should expose 2 variants:

  1. PREFERRED: Add a convenience command that will download the fluvio/dedup-bloom-filter@0.1.0 smartmodule automatically.

    fluvio topic create dedup_topic --dedup
fluvio topic create dedup_topic --dedup --dedup-age 1h --dedup-count 5

  2. ADVANCED: Expose the same --smartmodule command that works with fluvio consume for the user to apply a user-defined smartmodules and parameters:

    fluvio hub sm download fluvio/dedup-bloom-filter@0.1.0
     fluvio topic create dedup_topic --smartmodule fluvio/dedup-bloom-filter@0.1.0
 fluvio topic create dedup_topic --smartmodule fluvio/dedup-bloom-filter@0.1.0 -e dedup-page=1h  -e dedup-count=5

Sure, I did it like the PREFERRED way, and I also created an issue for the ADVANCED way

@fraidev fraidev requested review from digikata and sehz February 11, 2025 17:53
@sehz
Copy link
Contributor

sehz commented Feb 11, 2025

Is there issue describing what new option does?

@sehz
Copy link
Contributor

sehz commented Feb 11, 2025 •
edited
Loading

Is there contract on smartmodule name and version? What if user doesn't download right version of smartmodule

sehz
sehz reviewed Feb 11, 2025
View reviewed changes
crates/fluvio-cli/src/client/topic/create.rs
@@ -258,6 +270,21 @@ fn validate(name: &str, _spec: &TopicSpec) -> Result<()> {
Ok(())
}

fn create_deduplication(dedup_count: u64, dedup_age: Option<Duration>) -> Deduplication {
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 be builder pattern so can customize this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that Deduplication, Bounds and Filter implements Builder, but seems a little verbose to create 3 Builders for this, no?

Maybe with additional methods at Deduplication?

@ajhunyady
Copy link
Contributor

Is there contract on smartmodule name and version? What if user doesn't download right version of smartmodule

As per my recommendation, there are 2 flavors (see above).
for the

  1. preferred - we should decide the version
  2. advanced - it' just like any other smartmodule, it's up to the user.

@sehz
Copy link
Contributor

sehz commented Feb 11, 2025 

--dedup --dedup-age 1h --dedup-count 5

Is this option applies to all deduplication smartmodule or just on specific version of smartmodule?

@sehz
Copy link
Contributor

sehz commented Feb 11, 2025

@fraidev
Let's have issue for it to iterate instead of here.

@fraidev
Copy link
Contributor Author

fraidev commented Feb 11, 2025 

--dedup --dedup-age 1h --dedup-count 5

Is this option applies to all deduplication smartmodule or just on specific version of smartmodule?

Specific version.
"fluvio/dedup-bloom-filter@0.1.0" for now.

@fraidev
Copy link
Contributor Author

fraidev commented Feb 11, 2025

@fraidev Let's have issue for it to iterate instead of here.

Sure, it's here: #4388

@fraidev fraidev force-pushed the dedup_cli branch 2 times, most recently from 8bbaffc to ec7ade7 Compare February 11, 2025 22:40
@fraidev fraidev requested a review from sehz February 11, 2025 22:42
@fraidev
Copy link
Contributor Author

fraidev commented Feb 11, 2025

@sehz I changed the PR to download the specific SM if not downloaded.

@fraidev fraidev changed the title feat: add deduplicate opt for create topic cli feat: add deduplicate parameter to create topic using CLI Feb 11, 2025
@fraidev fraidev changed the title feat: add deduplicate parameter to create topic using CLI feat: add deduplicate parameter to create topic using cli Feb 11, 2025
@fraidev
Copy link
Contributor Author

fraidev commented Feb 25, 2025

@ajhunyady and @sehz do we still have any blockers here?

sehz
sehz reviewed Feb 25, 2025
View reviewed changes
tests/cli/fluvio_smoke_tests/topic-basic.bats
@@ -23,7 +23,7 @@ setup_file() {
TOPIC_NAME_SYSTEM=$(random_string)
export TOPIC_NAME_SYSTEM

DEDUP_FILTER_NAME="dedup-filter"
DEDUP_FILTER_NAME="dedup-bloom-filter"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there integration or CLI test?

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 just a test creating a sm. I'll create cli tests for the "--dedup" then

Copy link
Contributor Author

@fraidev fraidev Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I added a cli test using --dedup

@fraidev fraidev force-pushed the dedup_cli branch 2 times, most recently from d71aeb9 to a7f50f8 Compare February 26, 2025 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sehz sehz sehz left review comments

@digikata digikata Awaiting requested review from digikata

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fraidev @ajhunyady @sehz