Support Create Partitions - #623
Conversation
|
Working on tests still but posting for initial feedback |
|
thank you for the PR @BlueCollarChris |
|
Just seeing the comments now and will get back to you on Monday. I had been exploring a different route locally where the sync is done outside the client in a linked genserver. I wasn't sure if the sync process were to block the client would be disrupted from handling other requests. I'll see about getting those changes up shortly. |
| interval = Interval | ||
| } = State | ||
| ) -> | ||
| sync_partitions(Client, Sup, Config), |
There was a problem hiding this comment.
This ensure the new partitions get a producer but then we need to tell the client to update the metadata so it can use the new partition.
| brod_supervisor3:start_child(SupPid, Spec). | ||
|
|
||
| %% @doc Dynamically start a per partition producer | ||
| -spec start_producer(pid(), pid(), brod:topic(), non_neg_integer(), brod:producer_config()) -> |
There was a problem hiding this comment.
variant for starting just the partition producer with the correct child spec
|
@zmstone got the update in I mentioned over the weekend. Is there a specific formatter you recommend looks like the one for my IDE formatted some stuff showing more changes than were there previously. I did make the change to start the partition producer under the brod_supervisor3 as mentioned in your previous comment. Working on the tests for the brod_partitions_sync module still. |
|
Hi @BlueCollarChris About code format: we use https://github.com/WhatsApp/erlfmt Possible to leave code reformat out in this PR? it makes review easier. |
|
I have been completely swamped but will get to updating the formatting of this very soon, sorry for the delay |
|
thanks. there are some conflicts. |
646aeb2 to
a9e1e9d
Compare
|
Working on the formatting at this point to remove the changes and conform the new module to it |
|
@zmstone This is now ready. I have removed the formatting changes from the code that was causing the delay from earlier. |
| %%% allout-layout: t | ||
| %%% erlang-indent-level: 2 | ||
| %%% End: | ||
| %%% End: No newline at end of file |
| %%% allout-layout: t | ||
| %%% erlang-indent-level: 2 | ||
| %%% End: | ||
| %%% End: No newline at end of file |
|
|
||
| %% Test cases | ||
| -export([ t_create_delete_topics/1 | ||
| -export([ t_create_update_delete_topics/1 |
There was a problem hiding this comment.
could you add another case which starts producers for a topic, then create new partitions, then assert that partition producer is started automatically?
| %% @doc A `brod_partitions_sync' is a `gen_server' that is responsible for fetching | ||
| %% the latest partition counts for a client and ensuring external changes to partitions | ||
| %% are propogated to the clients and starts a producer for the partition if not present | ||
| -module(brod_partitions_sync). |
There was a problem hiding this comment.
there is maybe no need for this module.
we can start a timer (ticker) in brod_client process to trigger a periodic metadata refresh.
brod_client will automatically start the producers for newly discovered partitions.
Lines 638 to 643 in 674ae4d
Add brod:create_partitions/3,4 to add partitions to an existing topic. Add optional client config metadata_refresh_interval_seconds: when set, the client periodically refreshes metadata for the topics it has producers running for, so producers are started automatically for newly discovered partitions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9c86899 to
875358d
Compare
|
Finally got around to closing this out, hope it is what was expected. Verified our forked version through my companies tests that the alt approach recommended handles our use case. |
2 participants
Adds the ability to create partitions for an existing topic (
brod:create_partitions/3,4), exposingcreate_partitionsfrom the underlyingkafka_protocol.Adds an optional client config
metadata_refresh_interval_seconds: when set, the client starts a ticker in thebrod_clientprocess that periodically refreshes metadata for the topics it has producers running for, so producers are started automatically for newly discovered partitions (via the existingmaybe_start_partition_producerpath added in #636).Reworked per review: the
brod_partitions_syncmodule is gone in favor of the in-client ticker, the branch is rebased on current master with the formatting noise dropped, andbrod_SUITEgainst_auto_start_producers_for_new_partitionswhich creates a topic, starts a producer, adds a partition, and asserts the new partition producer starts automatically.