Skip to content

Update 00170_s3_cache.sql#36743

Merged
kssenii merged 4 commits intomasterfrom
kssenii-patch-3
Apr 28, 2022
Merged

Update 00170_s3_cache.sql#36743
kssenii merged 4 commits intomasterfrom
kssenii-patch-3

Conversation

@kssenii
Copy link
Copy Markdown
Member

@kssenii kssenii commented Apr 28, 2022

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

kssenii
kssenii commented Apr 28, 2022
View reviewed changes
tests/queries/1_stateful/00170_s3_cache.sql

-- { echo }

SET enable_filesystem_cache_on_write_operations=0;
Copy link
Copy Markdown
Member Author

@kssenii kssenii Apr 28, 2022

Choose a reason for hiding this comment

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

Will change back to 1, when #36171 is merged.
Disabling this setting for now because looks like (not really sure) it might cause hung check in stress test.
This should be fixed in #36171 as it makes write-through cache easier and safer to use.

Copy link
Copy Markdown
Contributor

@KinderRiven KinderRiven Apr 28, 2022

Choose a reason for hiding this comment

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

Will change back to 1, when #36171 is merged. Disabling this setting for now because looks like (not really sure) it might cause hung check in stress test. This should be fixed in #36171 as it makes write-through cache easier and safer to use.

00170_s3_cache.reference should also be modify? Otherwise it may fail the test？

Copy link
Copy Markdown
Member Author

@kssenii kssenii Apr 28, 2022

Choose a reason for hiding this comment

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

The .reference of this test will not be affected after disabling this setting.
But thanks for suggesting.

Copy link
Copy Markdown
Contributor

@KinderRiven KinderRiven Apr 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The .reference of this test will not be affected after disabling this setting. But thanks for suggesting.

Really? The result may not be affected, but it will be output this SQL instruction in reference file.

Copy link
Copy Markdown
Member Author

@kssenii kssenii Apr 28, 2022

Choose a reason for hiding this comment

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

ah indeed!

@robot-ch-test-poll robot-ch-test-poll added the pr-not-for-changelog This PR should not be mentioned in the changelog label Apr 28, 2022
@kssenii kssenii mentioned this pull request Apr 28, 2022
Merged
@kssenii
Copy link
Copy Markdown
Member Author

kssenii commented Apr 28, 2022

@Mergifyio update

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Apr 28, 2022

update

✅ Branch has been successfully updated

@kssenii
Copy link
Copy Markdown
Member Author

kssenii commented Apr 28, 2022

Stress test: #36660

@kssenii kssenii merged commit 2f64255 into master Apr 28, 2022
@kssenii kssenii deleted the kssenii-patch-3 branch April 28, 2022 22:03
@kssenii kssenii mentioned this pull request Apr 29, 2022
Merged
This was referenced Apr 29, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qoega qoega qoega approved these changes

+1 more reviewer

@KinderRiven KinderRiven KinderRiven left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kssenii @qoega @KinderRiven @robot-ch-test-poll