Skip to content

Fix for parallel replicas not completely disabled by granule count threshold#51805

Merged
alexey-milovidov merged 3 commits intomasterfrom
fix_for_parallel_replicas_and_empty_header
Jul 5, 2023
Merged

Fix for parallel replicas not completely disabled by granule count threshold#51805
alexey-milovidov merged 3 commits intomasterfrom
fix_for_parallel_replicas_and_empty_header

Conversation

@davenger
Copy link
Copy Markdown
Member

@davenger davenger commented Jul 4, 2023

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

This led to incorrect result when trying to read 0 columns from remote for count()

@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jul 4, 2023
@robot-ch-test-poll1
Copy link
Copy Markdown
Contributor

robot-ch-test-poll1 commented Jul 4, 2023 •
edited
Loading

This is an automated comment for commit 9cadcb1 with description of existing statuses. It's updated for the latest CI running
The full report is available here
The overall status of the commit is 🟡 pending

Check nameDescriptionStatus
CI runningA meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR🟡 pending
Mergeable CheckChecks if all other necessary checks are successful🟢 success

@davenger davenger changed the title Fix for parallel replicas not completely disabled when by granulel threshold Fix for parallel replicas not completely disabled by granule count threshold Jul 4, 2023
@davenger davenger force-pushed the fix_for_parallel_replicas_and_empty_header branch from ef15c11 to c0629a3 Compare July 4, 2023 21:50
@Algunenano
Copy link
Copy Markdown
Member

Algunenano commented Jul 5, 2023

I guess this will conflict with #51805, which fully deactivates parallel replicas (no setup) instead.

@davenger
Copy link
Copy Markdown
Member Author

davenger commented Jul 5, 2023

I guess this will conflict with #51805, which fully deactivates parallel replicas (no setup) instead.

@Algunenano you probably meant not #51805 (it's this one) but some other PR?

@Algunenano
Copy link
Copy Markdown
Member

Algunenano commented Jul 5, 2023

Yes sorry, I meant #51692

@davenger
Copy link
Copy Markdown
Member Author

davenger commented Jul 5, 2023

Yes sorry, I meant #51692

@Algunenano I tried you PR and it does fix my case.
But looks like the PR is still work in progress. I think it makes sense to commit my quick fix and when you finish your work it will replace my fix.

@Algunenano
Copy link
Copy Markdown
Member

Algunenano commented Jul 5, 2023

But looks like the PR is still work in progress. I think it makes sense to commit my quick fix and when you finish your work it will replace my fix.

As you wish, I can simply revert the commits (to remove the new condition) once this is merged. If you want to continue I'd suggest also testing with ReplicatedMergeTree, I'd expect to see a similar condition also in StorageReplicatedMergeTree::read (but I haven't tested if it's necessary).

@alexey-milovidov alexey-milovidov self-assigned this Jul 5, 2023
@alexey-milovidov alexey-milovidov merged commit 2c592a6 into master Jul 5, 2023
@alexey-milovidov alexey-milovidov deleted the fix_for_parallel_replicas_and_empty_header branch July 5, 2023 22:46
Algunenano added a commit to Algunenano/ClickHouse that referenced this pull request Jul 11, 2023
@Algunenano
Revert "Merge pull request ClickHouse#51805 from ClickHouse/fix_for_p…
9a5f357 
…arallel_replicas_and_empty_header"

This reverts commit 2c592a6, reversing
changes made to 7a593ed.
Algunenano added a commit to Algunenano/ClickHouse that referenced this pull request Jul 11, 2023
@Algunenano
Add test from ClickHouse#51805
35b315b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees

@alexey-milovidov alexey-milovidov

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

@davenger @robot-ch-test-poll1 @Algunenano @alexey-milovidov