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

feat: improve topic creation api #528

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

Conversation

fraidev
Copy link
Contributor

@fraidev fraidev commented Dec 12, 2024

Simple (1 partition and 1 replica):

fluvio_admin = FluvioAdmin.connect()
fluvio_admin.create_topic("topic_name")

or

fluvio_admin = FluvioAdmin.connect()
topic_spec = TopicSpec.new()
fluvio_admin.create_topic("topic_name", topic_spec)

Advanced:

fluvio_admin = FluvioAdmin.connect()
topic_spec = (
    TopicSpec.create()
    .with_partitions(3)
    .with_replications(3)
    .with_max_partition_size("1Gb")
    .with_retention_time(3600)
    .with_segment_size("10M")
    .as_system_topic()
    .build()
)
fluvio_admin.create_topic("topic_name", topic_spec)

I am using NewTopic only because kafka-python also uses a class called NewTopic

@fraidev fraidev force-pushed the improve_topic_creation branch 6 times, most recently from b44aa29 to a82fbe9 Compare December 12, 2024 04:23
@fraidev fraidev requested a review from digikata December 12, 2024 04:36
@fraidev fraidev force-pushed the improve_topic_creation branch from a82fbe9 to cd3375c Compare December 12, 2024 04:37
@digikata
Copy link
Contributor

digikata commented Dec 12, 2024

Instead of using Kafka terms, lets stay with the terminology in the Rust crate, TopicSpec. PyO3 lets us provide good inline python docs, but if those are insufficient, people (including ourselves) often end up referring to the Rust crate code. So staying consistent with that is a principle.

from humanfriendly import parse_timespan


class TopicMode(Enum):
Copy link

Choose a reason for hiding this comment

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

Instead of creating python enum, should use derive macro like here: https://pyo3.rs/v0.23.2/conversions/traits.html

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 new enum is only for the builder of TopicSpec, if we consider the solution without the build it's not even needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the Compression Enum is worth, thought

@fraidev
Copy link
Contributor Author

fraidev commented Dec 12, 2024

Instead of using Kafka terms, lets stay with the terminology in the Rust crate, TopicSpec. PyO3 lets us provide good inline python docs, but if those are insufficient, people (including ourselves) often end up referring to the Rust crate code. So staying consistent with that is a principle.

Sure, but should I merge them or rename the actual TopicSpec?

Because the actual TopicSpec is a wrapper of the inner TopicSpec from Rust.

The NewTopic that I created is the builder of TopicSpec. Allowing specifying the number of the partitions and replications with these methods, instead just call new_computed method.

Merge them would be something like this, what do you think?

fluvio_admin = FluvioAdmin.connect()
topic_spec = (
    TopicSpec.new_computed(partitions=3, replications=3, ignore=True)
    .with_max_partition_size("1Gb")
    .with_retention_time(3600)
    .with_segment_size("10M")
    .as_system_topic()
)
fluvio_admin.create_topic(self.topic, topic_spec)

Another alternative is rename NewTopic to TopicSpecBuilder or rename the NewTopic to TopicSpec and TopicSpec to something else.

fluvio_admin = FluvioAdmin.connect()
topic_spec = (
    TopicSpecBuilder.create()
    .with_partitions(3)
    .with_replications(3)
    .with_max_partition_size("1Gb")
    .with_retention_time(3600)
    .with_segment_size("10M")
    .as_system_topic()
    .build()
)
fluvio_admin.create_topic(self.topic, topic_spec)

@digikata
Copy link
Contributor

The first one is good w/ TopicSpec. If you can do it with the direct annotations in lib.rs PyO3 or appropriate module use that. There are a lot of instances in the fluvio python client that rename some FluvioStruct to _FluvioStruct internally, so a pythonic FluvioStruct can be supplied to the interpreter too, but it's extra layers that should have some interface value if thats done. (there are legacy portions of the code that does it unecessarily)

@digikata
Copy link
Contributor

Oh we shoudl also remove the required partitions, replications, and bool arguments and just support TopicSpec.new() with defaults that can be changed w/ the builder style calls as you did with the other parameters

@fraidev
Copy link
Contributor Author

fraidev commented Dec 13, 2024

Oh we shoudl also remove the required partitions, replications, and bool arguments and just support TopicSpec.new() with defaults that can be changed w/ the builder style calls as you did with the other parameters

I think that would need a larger refactor on fluvio side. IIRC, we don't have the feature of adding replicas to an already existent Topic.

In the builder it was easier because it's calling new only after the build function.

wdyt?

@digikata
Copy link
Contributor

This is only more featured topic create for the python client, if the topic already exists the create should return an error.

@fraidev fraidev force-pushed the improve_topic_creation branch 3 times, most recently from aed561b to 2db7e99 Compare December 14, 2024 01:10
self.inner.set_compression_type(CompressionAlgorithm::Zstd);
}
self.inner.set_compression_type(CompressionAlgorithm::Any);
}
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 probably return an error if not recognized, or it could accept a compression type enum

Copy link
Contributor Author

@fraidev fraidev Dec 14, 2024

Choose a reason for hiding this comment

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

I changed to return result, and an error for this case.
The support for enum that are not tag unions does not seem great in pyo3, it's also not automatically typed, so no benefits at all.

@fraidev fraidev force-pushed the improve_topic_creation branch from 2db7e99 to c5a89b6 Compare December 14, 2024 02:21
@fraidev fraidev requested review from digikata and sehz December 14, 2024 03:35
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