Skip to content

Revert "Move logging to a separate thread"#82068

Merged
azat merged 1 commit intomasterfrom
revert-80125-poco_async_logger
Jun 18, 2025
Merged

Revert "Move logging to a separate thread"#82068
azat merged 1 commit intomasterfrom
revert-80125-poco_async_logger

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented Jun 17, 2025

Reverts #80125

Refs: https://pastila.nl/?0024bdfc/d647932af97f68f225dffef20154f102#aF2+D7zvdKZfYBT3/baJ2w==

Otherwise it breaks some tests, i.e. #81916

And just disabling async logging (#82034), triggers other problems on CI - #82059

@azat azat requested a review from Algunenano June 17, 2025 20:08
@azat azat mentioned this pull request Jun 17, 2025
Closed
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jun 17, 2025 •
edited
Loading

Workflow [PR], commit [7a75cf7]

Summary: ❌

job_name test_name status info comment
Performance Comparison (amd_release, master_head, 2/3) failure
Check Results failure

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jun 17, 2025
@azat azat enabled auto-merge June 18, 2025 04:59
@azat
Copy link
Copy Markdown
Member Author

azat commented Jun 18, 2025

Performance Comparison (amd_release, master_head, 2/3) — 10 errors, 2 too long, 2 faster, 2 unstable

Traceback (most recent call last):
  File "/home/ubuntu/actions-runner/_work/ClickHouse/ClickHouse/./tests/performance/scripts/perf.py", line 441, in <module>
    res = c.execute(
  File "/usr/local/lib/python3.10/dist-packages/clickhouse_driver/client.py", line 382, in execute
    rv = self.process_ordinary_query(
  File "/usr/local/lib/python3.10/dist-packages/clickhouse_driver/client.py", line 580, in process_ordinary_query
    return self.receive_result(with_column_types=with_column_types,
  File "/usr/local/lib/python3.10/dist-packages/clickhouse_driver/client.py", line 213, in receive_result
    return result.get_result()
  File "/usr/local/lib/python3.10/dist-packages/clickhouse_driver/result.py", line 50, in get_result
    for packet in self.packet_generator:
  File "/usr/local/lib/python3.10/dist-packages/clickhouse_driver/client.py", line 229, in packet_generator
    packet = self.receive_packet()
  File "/usr/local/lib/python3.10/dist-packages/clickhouse_driver/client.py", line 246, in receive_packet
    raise packet.exception
clickhouse_driver.errors.ServerException: Code: 159.
countMatches.query4.run0: DB::Exception: Timeout exceeded: elapsed 15032.275923 ms, maximum: 15000 ms. Stack trace:

0. DB::Exception::Exception(DB::Exception::MessageMasked&&, int, bool) @ 0x0000000010157a3b
1. DB::Exception::Exception(PreformattedMessage&&, int) @ 0x0000000009d5970c
2. DB::Exception::Exception<String&, unsigned long const&>(int, FormatStringHelperImpl<std::type_identity<String&>::type, std::type_identity<unsigned long const&>::type>, String&, unsigned long const&) @ 0x00000000142c7a8b
3. DB::QueryStatus::throwProperExceptionIfNeeded(unsigned long const&, unsigned long const&) @ 0x00000000144c1312
4. DB::PipelineExecutor::finalizeExecution() @ 0x0000000015c36c63
5. DB::PipelineExecutor::execute(unsigned long, bool) @ 0x0000000015c367bd
6. void std::__function::__policy_invoker<void ()>::__call_impl[abi:ne190107]<std::__function::__default_alloc_func<ThreadFromGlobalPoolImpl<true, true>::ThreadFromGlobalPoolImpl<DB::PullingAsyncPipelineExecutor::pull(DB::Chunk&, unsigned long)::$_0>(DB::PullingAsyncPipelineExecutor::pull(DB::Chunk&, unsigned long)::$_0&&)::'lambda'(), void ()>>(std::__function::__policy_storage const*) @ 0x0000000015c4922e
7. ThreadPoolImpl<std::thread>::ThreadFromThreadPool::worker() @ 0x000000001029f9f2
8. void* std::__thread_proxy[abi:ne190107]<std::tuple<std::unique_ptr<std::__thread_struct, std::default_delete<std::__thread_struct>>, void (ThreadPoolImpl<std::thread>::ThreadFromThreadPool::*)(), ThreadPoolImpl<std::thread>::ThreadFromThreadPool*>>(void*) @ 0x00000000102a6eba
9. start_thread @ 0x0000000000094ac3
10. __clone @ 0x0000000000125a04

@azat azat added this pull request to the merge queue Jun 18, 2025
Merged via the queue into master with commit 8bd9858 Jun 18, 2025
119 of 122 checks passed
@azat azat deleted the revert-80125-poco_async_logger branch June 18, 2025 05:36
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 18, 2025
@Algunenano
Copy link
Copy Markdown
Member

Algunenano commented Jun 23, 2025

Reverting also in 25.6. I'll add it back if everything is fixed, if not it will go into 25.7

@robot-ch-test-poll1 robot-ch-test-poll1 mentioned this pull request Jun 23, 2025
Merged
robot-ch-test-poll1 added a commit that referenced this pull request Jun 23, 2025
@robot-ch-test-poll1
Merge pull request #82392 from ClickHouse/cherrypick/25.6/82068
28eca4b 
Cherry pick #82068 to 25.6: Revert "Move logging to a separate thread"
robot-clickhouse added a commit that referenced this pull request Jun 23, 2025
@robot-clickhouse
Backport #82068 to 25.6: Revert "Move logging to a separate thread"
659fb56
@robot-ch-test-poll1 robot-ch-test-poll1 mentioned this pull request Jun 23, 2025
Merged
@robot-clickhouse robot-clickhouse added the pr-backports-created-cloud deprecated label, NOOP label Jun 23, 2025
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Jun 23, 2025
Algunenano added a commit that referenced this pull request Jun 23, 2025
@Algunenano
Merge pull request #82393 from ClickHouse/backport/25.6/82068
499893a 
Backport #82068 to 25.6: Revert "Move logging to a separate thread"
Algunenano
Algunenano reviewed Jun 24, 2025
View reviewed changes
src/Common/Exception.cpp
try
{
if (logged != nullptr && !logged->load(std::memory_order_relaxed) && isErrorCodeImportant())
if (logged != nullptr && !logged->load(std::memory_order_relaxed) && isErrorCodeImportant() && isLoggingEnabled())
Copy link
Copy Markdown
Member

@Algunenano Algunenano Jun 24, 2025

Choose a reason for hiding this comment

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

This is what was preventing the "race condition" in #82059 due to different static object destruction order from appearing before. Needs to be added back in the revert-revert PR or add something similar

Algunenano added a commit to Algunenano/ClickHouse that referenced this pull request Jun 24, 2025
@Algunenano
Revert "Merge pull request ClickHouse#82068 from ClickHouse/revert-80…
0810984 
…125-poco_async_logger"

This reverts commit 8bd9858, reversing
changes made to 40e0202.
@Algunenano Algunenano mentioned this pull request Jun 24, 2025
Merged
1 task
@robot-clickhouse robot-clickhouse added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Algunenano Algunenano Algunenano left review comments

Assignees

No one assigned

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo v25.6-must-backport

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@azat @Algunenano @robot-clickhouse @robot-ch-test-poll4 @robot-ch-test-poll3