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

[cloud_storage] segment_meta_cstore::materialize: revert find to lower_bound #11404

Merged

Conversation

andijcr
Copy link
Contributor

@andijcr andijcr commented Jun 13, 2023

fix a previous wrong commit.
the proper way to use the _hints vector is with lower_bound, to get an exact match or a close one, to speed up materialize. with just find, most of the call will take the slow path

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.1.x
  • v22.3.x
  • v22.2.x

Release Notes

  • none

Comment on lines -552 to +555
auto hint_it = _hints.find(bo);
auto hint_it = _hints.lower_bound(bo);
Copy link
Member

@dotnwat dotnwat Jun 14, 2023

Choose a reason for hiding this comment

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

seems like maybe we should have a unit test for the case that caught this issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's difficult to model since it's just an optimization for faster lookup. I can see if there is a significant time discrepancy, but it would require some code to work as a baseline.

probably it could be easier to expose some internals to the test, maybe via a friend declaration

@andijcr
Copy link
Contributor Author

andijcr commented Jun 14, 2023

/ci-repeat 3 dt-repeat=3 tests/rptest/tests/offset_retention_test.py::OffsetRetentionTest.test_offset_expiration

@andijcr andijcr force-pushed the fix/segment_meta_cstore/materialize branch from 09080d7 to 37e9e46 Compare June 27, 2023 14:23
@andijcr
Copy link
Contributor Author

andijcr commented Jun 27, 2023

force push: rebase on dev and ci run

@andijcr
Copy link
Contributor Author

andijcr commented Jun 27, 2023

failure is #11455 and #11182

@andijcr
Copy link
Contributor Author

andijcr commented Jun 27, 2023

/ci-repeat

@andijcr
Copy link
Contributor Author

andijcr commented Jun 28, 2023

https://buildkite.com/redpanda/redpanda/builds/32087#0188fe8b-7fe5-45a0-baa9-3717421e98e1
issues are
#11449
and
new issue fixed by #11733

Doing a ci repeat for the offending test

@andijcr
Copy link
Contributor Author

andijcr commented Jun 28, 2023

/ci-repeat 3 skip-unit dt-repeat=10 tests/rptest/tests/topic_delete_test.py::TopicDeleteCloudStorageTest.topic_delete_unavailable_test
tests/rptest/tests/topic_creation_test.py::CreateTopicUpgradeTest.test_retention_config_on_upgrade_from_v22_2_to_v22_3

fix a previous wrong commit.
the proper way to use the _hints vector is with lower_bound, to get an
exact match or a close one, to speed up materialize. with just find,
most of the call will take the slow path
@andijcr andijcr force-pushed the fix/segment_meta_cstore/materialize branch from 37e9e46 to 94a86c9 Compare June 30, 2023 08:46
@andijcr
Copy link
Contributor Author

andijcr commented Jun 30, 2023

force push: rebase on dev to check for conflicts

Copy link
Contributor

@Lazin Lazin left a comment

Choose a reason for hiding this comment

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

LGTM!

@andijcr
Copy link
Contributor Author

andijcr commented Jun 30, 2023

https://buildkite.com/redpanda/redpanda/builds/32265
issues:
#11362
#11790

and a new one

====================================================================================================
test_id:    rptest.tests.cloud_storage_timing_stress_test.CloudStorageTimingStressTest.test_cloud_storage_with_partition_moves.cleanup_policy=compact.delete
status:     FAIL
run time:   2 minutes 25.957 seconds


    TimeoutError('Workload did not complete within 120s: Consumer consumed only 11259 out of 15360 messages')
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/dist-packages/ducktape/tests/runner_client.py", line 135, in run
    data = self.run_test()
  File "/usr/local/lib/python3.10/dist-packages/ducktape/tests/runner_client.py", line 227, in run_test
    return self.test_context.function(self.test)
  File "/usr/local/lib/python3.10/dist-packages/ducktape/mark/_mark.py", line 481, in wrapper
    return functools.partial(f, *args, **kwargs)(*w_args, **w_kwargs)
  File "/root/tests/rptest/services/cluster.py", line 79, in wrapped
    r = f(self, *args, **kwargs)
  File "/root/tests/rptest/utils/mode_checks.py", line 63, in f
    return func(*args, **kwargs)
  File "/root/tests/rptest/tests/cloud_storage_timing_stress_test.py", line 527, in test_cloud_storage_with_partition_moves
    while not self.is_complete():
  File "/root/tests/rptest/tests/cloud_storage_timing_stress_test.py", line 362, in is_complete
    raise TimeoutError(
TimeoutError: Workload did not complete within 120s: Consumer consumed only 11259 out of 15360 messages

repeating this last one

@andijcr
Copy link
Contributor Author

andijcr commented Jun 30, 2023

/ci-repeat 3 release skip-unit dt-repeat=5 tests/rptest/tests/cloud_storage_timing_stress_test.py::CloudStorageTimingStressTest.test_cloud_storage_with_partition_moves

1 similar comment
@andijcr
Copy link
Contributor Author

andijcr commented Jul 3, 2023

/ci-repeat 3 release skip-unit dt-repeat=5 tests/rptest/tests/cloud_storage_timing_stress_test.py::CloudStorageTimingStressTest.test_cloud_storage_with_partition_moves

@andijcr andijcr merged commit 372819b into redpanda-data:dev Jul 3, 2023
17 checks passed
@andijcr andijcr deleted the fix/segment_meta_cstore/materialize branch July 3, 2023 12:52
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.

3 participants