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

fix: link_parent more realible and delete topic partitions #4219

Merged
merged 2 commits into from
Oct 29, 2024

Conversation

fraidev
Copy link
Contributor

@fraidev fraidev commented Oct 26, 2024

The fix of #4094 was a wrong fix to have sure that all partitions are childrens of its Topic.

I reverted this PR, and I made the link_parent and unlink_parent more reliable.
Because they were being called correct, but link_parent it was using stale objects.

Now they are getting the parent from the store to have sure to not lose data.

How to test:

fluvio topic create data
fluvio topic add-partition data
fluvio topic add-partition data
fluvio topic add-partition data
fluvio topic add-partition data
fluvio topic add-partition data
fluvio topic delete data
fluvio partition list

Must output no results.

This should also fix logs like these after each 5min (SC_RECONCILIATION_INTERVAL_SEC):

 sc        | 2024-10-25T21:16:09.364724Z ERROR MetadataDispatcher{spec="Partition" namespace="default"}:process_ws_action: fluvio_stream_dispatcher::dispatcher::metadata: error: Partition, applying attempt to update by stale value: current version: 11, proposed: 10
sc        | 2024-10-25T21:16:09.366955Z ERROR MetadataDispatcher{spec="Partition" namespace="default"}:process_ws_action: fluvio_stream_dispatcher::dispatcher::metadata: error: Partition, applying attempt to update by stale value: current version: 5, proposed: 4
sc        | 2024-10-25T21:16:09.370491Z ERROR MetadataDispatcher{spec="Partition" namespace="default"}:process_ws_action: fluvio_stream_dispatcher::dispatcher::metadata: error: Partition, applying attempt to update by stale value: current version: 14, proposed: 13
sc        | 2024-10-25T21:16:09.378256Z ERROR MetadataDispatcher{spec="Partition" namespace="default"}:process_ws_action: fluvio_stream_dispatcher::dispatcher::metadata: error: Partition, applying attempt to update by stale value: current version: 8, proposed: 7
sc        | 2024-10-25T21:16:09.379879Z ERROR MetadataDispatcher{spec="Partition" namespace="default"}:process_ws_action: fluvio_stream_dispatcher::dispatcher::metadata: error: Partition, applying attempt to update by stale value: current version: 8, proposed: 7
sc        | 2024-10-25T21:16:09.381140Z ERROR MetadataDispatcher{spec="Partition" namespace="default"}:process_ws_action: fluvio_stream_dispatcher::dispatcher::metadata: error: Partition, applying attempt to update by stale value: current version: 6, proposed: 5
sc        | 2024-10-25T21:16:09.382941Z ERROR MetadataDispatcher{spec="Partition" namespace="default"}:process_ws_action: fluvio_stream_dispatcher::dispatcher::metadata: error: Partition, applying attempt to update by stale value: current version: 12, proposed: 11

@fraidev fraidev changed the title fix: dispatch old partitions fix: link_parent more realible Oct 26, 2024
@fraidev fraidev changed the title fix: link_parent more realible fix: link_parent more realible and delete topic partitions Oct 26, 2024
@fraidev fraidev marked this pull request as ready for review October 26, 2024 21:14
@fraidev fraidev force-pushed the fix_dispacher branch 6 times, most recently from 7e4bb89 to 40c8f1a Compare October 27, 2024 03:16
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.

Fix seems to look good. Can you add CLI Test as described in the issue?

fluvio topic create data
fluvio topic add-partition data
fluvio topic add-partition data
fluvio topic add-partition data
fluvio topic add-partition data
fluvio topic add-partition data
fluvio topic delete data
fluvio partition list

@fraidev
Copy link
Contributor Author

fraidev commented Oct 27, 2024

Fix seems to look good. Can you add CLI Test as described in the issue?

fluvio topic create data
fluvio topic add-partition data
fluvio topic add-partition data
fluvio topic add-partition data
fluvio topic add-partition data
fluvio topic add-partition data
fluvio topic delete data
fluvio partition list

I created one at #4094 at tests/cli/fluvio_smoke_tests/add-partitions.bats.

@sehz
Copy link
Contributor

sehz commented Oct 27, 2024

But then that didn't catch error. Can you create PR to reproduce bug?

@fraidev
Copy link
Contributor Author

fraidev commented Oct 27, 2024

But then that didn't catch error. Can you create PR to reproduce bug?

The #4094 fixed this bug with changes at crates/fluvio-sc/src/stores/topic/store.rs. (In a wrong way)
I reverted these changes there, and fixed it with changes in crates/fluvio-stream-dispatcher/src/metadata/local.rs

I can create a PR with only crates/fluvio-sc/src/stores/topic/store.rs reverted, this test should catch it than. What do you think?

@sehz
Copy link
Contributor

sehz commented Oct 27, 2024

Let's not revert anything. This PR should be suffice for fix but we should make sure we can reproduce and confirm fix is applied. Since this bug can be manually reproduced, let's create PR to reproduce the bug (test should confirm bug). Once that is in, this PR should update test to assert bug fix is working

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

@fraidev fraidev added this pull request to the merge queue Oct 28, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 28, 2024
@fraidev fraidev added this pull request to the merge queue Oct 29, 2024
Merged via the queue into infinyon:master with commit 573751b Oct 29, 2024
106 checks passed
@fraidev fraidev deleted the fix_dispacher branch October 29, 2024 03:52
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.

2 participants