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

Zero copy tests #251

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Zero copy tests #251

merged 1 commit into from
Nov 1, 2024

Conversation

joelynch
Copy link
Contributor

  • Set zero copy settings in tests.
  • make sure object storage test table has data both on local and remote disk.

@joelynch joelynch requested a review from a team October 25, 2024 16:11
@@ -313,6 +313,10 @@ async def setup_cluster_content(clients: Sequence[HttpClickHouseClient], clickho
await clients[0].execute(b"INSERT INTO default.in_object_storage VALUES (123, 'foo')")
await clients[1].execute(b"INSERT INTO default.in_object_storage VALUES (456, 'bar')")
await clients[2].execute(b"INSERT INTO default.in_object_storage VALUES (789, 'baz')")
# these should be routed to local disk
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would they be routed to the local disk?
Even if there is a reason, it is better to test. (check if system.parts has parts on all disks for example)

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 don't think this is actually needed to reproduce the bugs I was seeing. Removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand what is happening here now.
Commit says: "make sure object storage test table has data both on local and remote disk", but there are 0 new tests in this pr, and everything looks to work just as it was. Why are you now sure that the data is stored on both disks during tests?

@joelynch joelynch force-pushed the joelynch/astacus-zero-copy branch from bcbaf05 to cbe4a0a Compare October 30, 2024 11:58
@joelynch joelynch requested a review from Khatskevich October 30, 2024 11:59
Set zero copy settings in tests.
@joelynch joelynch force-pushed the joelynch/astacus-zero-copy branch from cbe4a0a to 0ca9a9a Compare October 31, 2024 09:18
@aris-aiven aris-aiven requested a review from a team October 31, 2024 15:47
@Khatskevich Khatskevich merged commit 02a3687 into main Nov 1, 2024
2 checks passed
@Khatskevich Khatskevich deleted the joelynch/astacus-zero-copy branch November 1, 2024 14:26
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