Try fix flaky tests with transactions by tavplubix · Pull Request #37822 · ClickHouse/ClickHouse

tavplubix · 2022-06-03T10:52:24Z

Changelog category (leave one):

Not for changelog (changelog entry is not required)

Previous PR #36644 fixed tests, but it triggers a bug in bash...
Let's try to simply send SIGTERM without any tricks
Also fixes #37920

This reverts commit 049d2a0.

tavplubix · 2022-06-03T11:24:34Z

@Mergifyio update

mergify · 2022-06-03T11:24:39Z

update

✅ Branch has been successfully updated

tavplubix · 2022-06-17T09:48:01Z

Tests with tsan - timeouts - broken after upgrading to llvm-14
Stateless tests flaky check (address, actions) - OK
Stress tests - #35156, suppressed in 299f102

tavplubix · 2022-06-20T10:17:29Z

Integration tests - No space left on device
Stateless tests with tsan are broken
Stateless tests flaky check - OK

But let's run tests again just in case

tavplubix · 2022-06-20T10:17:46Z

@Mergifyio update

mergify · 2022-06-20T10:17:51Z

update

✅ Branch has been successfully updated

tavplubix · 2022-06-20T18:10:59Z

Integration tests (asan, actions) [1/3] - No space left on device
Integration tests (release, actions) [2/2] - no space left on device
Integration tests (thread, actions) [4/4] - no space left on device

Stateless tests (aarch64, actions) - 02232_dist_insert_send_logs_level_hung - #38219
Stateless tests (address, actions) [1/2] - 02232_dist_insert_send_logs_level_hung
Stateless tests (debug, actions) [3/3] - 02232_dist_insert_send_logs_level_hung
Stateless tests (memory, actions) [3/3] - 02232_dist_insert_send_logs_level_hung
Stateless tests (release, DatabaseReplicated, actions) [1/2] - 02232_dist_insert_send_logs_level_hung
Stateless tests (release, actions) - 02232_dist_insert_send_logs_level_hung and 01281_group_by_limit_memory_tracking
Stateless tests (release, s3 storage, actions) - 02232_dist_insert_send_logs_level_hung
Stateless tests (release, wide parts enabled, actions) - 02232_dist_insert_send_logs_level_hung
Stateless tests (thread, actions) [3/3] - 02232_dist_insert_send_logs_level_hung
Stateless tests (ubsan, actions) - 02232_dist_insert_send_logs_level_hung

Stateless tests flaky check (address, actions) - OK

Stress test (debug, actions) and Stress test (thread, actions):

2022-06-20 16:54:46,552 Checking if some queries hung
usage: clickhouse-test [-h] [-q QUERIES] [--tmp TMP] [-b BINARY] [-c CLIENT]
...
clickhouse-test: error: unrecognized arguments: --stress

Some inconsistency in docker images (race condition in CI, cc: @Felixoid)

tavplubix · 2022-06-20T18:11:26Z

@Mergifyio update

mergify · 2022-06-20T18:11:30Z

update

✅ Branch has been successfully updated

azat · 2022-07-08T18:52:21Z

src/Common/noexcept_scope.h

+            assert((expected) == uncaught || (expected) + 1 == uncaught);          \
+            if ((expected) < uncaught)                                             \
+            {                                                                      \
+                tryLogCurrentException("NOEXCEPT_SCOPE");                          \


This will not work, you cannot rethrow exception outside the catch block, this will simply terminate (via std::terminate) the program. So this NOEXCEPT_SCOPE macro will simply call std::terminate on exception and will lost original exception.

I was thinking about this a little, and the only thing that pops up in my mind is setting some flag here and check it in Exception ctor and if it set, call abort/std::terminate, that way you will not loose it, however it looks not great.

You're right, it does not work as expected and seems like it's there's no way to get exception that is currently unwinding stack (even using __cxa_* functions or something from libunwind...). Not sure what options is better: just remove this like or add thread-local flag and check it when throwing an exception. Maybe it's ok to simply remove it for now and think about logging if we will face some issue that we will not be able to debug without information about unexpected exception...

I have another idea on how NOEXCEPT_SCOPE can be implemented, by accepting the whole code that should be executed inside, like SCOPE_EXIT, and this will also make it more explicit then simply defining in some scope.

What do you think about this? In the meantime I will prepare a patch and see how it will looks like.

Here is an RFC - #39229

azat · 2022-07-08T18:53:23Z

src/Storages/MergeTree/MergeTreeData.cpp

+            }
+        }
+
+        NOEXCEPT_SCOPE;


Can you please explain why it is required? And looks like it is better to add a comment for users of NOEXCEPT_SCOPE, thoughts?

See the comment:

ClickHouse/src/Common/noexcept_scope.h

Lines 30 to 33 in 39c0219

/// It can be used in critical places to exit on unexpected exceptions.

/// SIGABRT is usually better that broken in-memory state with unpredictable consequences.

/// It also temporarily disables exception from memory tracker in current thread.

/// Strict version does not take into account nested exception (i.e. it aborts even when we're in catch block).

It's required in MergeTreeData::Transaction::commit because changes to precommitted parts and MergeTreeData state (like set of parts with their states, indexes and column sizes) are not undone on exceptions. Normally this code should not throw any exceptions (unless maybe MEMORY_LIMIT_EXCEEDED, but exceptions from MemoryTracker are blocked), but in case it does (due to some bug or whatever) I would prefer to terminate immediately.

You can think of this macro as kind of paranoid defensive programming :)

Yep, I read that comment, and also came to conclusion that it is for counters accounting.

However I wasn't came to this PR randomly, I got some exception here, but due to NOEXCEPT_SCOPE the exception was not printed so I cannot know the exact reason, and zero information in logs. I've tried to get something from coredump but failed.

I've added a patch on top my build that will not loose the exception, however no exceptions occurred so far. I have one guess about exception there, that time zookeeper was under pressure, but this has been fixed before deploying the patch, and I'm not talking about some exception for zookeeper commands but instead some issues with parts maybe (some LOGICAL_ERROR).

azat · 2022-07-08T18:54:26Z

src/Interpreters/TransactionLog.cpp


-    NOEXCEPT_SCOPE;
-    LockMemoryExceptionInThread lock_memory_tracker(VariableContext::Global);
+    NOEXCEPT_SCOPE_STRICT;


I see the difference between them, but can you explain why strict variant is used here?

Because initially we had only strict variant and it was the only place where this macro was used. But strict variant is not generic enough to use it in other places. I decided to leave strict variant here because something is probably going wrong if some code tries to initialize transaction log during stack unwinding.

tavplubix added 2 commits June 3, 2022 12:29

Revert "fix tests"

63eea42

This reverts commit 049d2a0.

another attempt to fix tests

ce5acdb

tavplubix mentioned this pull request Jun 3, 2022

Try to fix some trash #37303

Merged

robot-ch-test-poll1 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jun 3, 2022

Merge branch 'master' into fix_flaky_tests_with_transactions

bcdeec1

azat mentioned this pull request Jun 7, 2022

Fix possible "No more packets are available" for distributed queries #37826

Merged

tavplubix added 2 commits June 14, 2022 12:07

Merge branch 'master' into fix_flaky_tests_with_transactions

86c8d0a

fix

b39bd3c

tavplubix force-pushed the fix_flaky_tests_with_transactions branch from 4868b96 to 1d5f43a Compare June 15, 2022 14:08

fix race on queue processing

e4d4d0a

tavplubix force-pushed the fix_flaky_tests_with_transactions branch from 1d5f43a to e4d4d0a Compare June 15, 2022 14:21

tavplubix added 3 commits June 15, 2022 20:47

Merge branch 'master' into fix_flaky_tests_with_transactions

5e2b42c

maybe fix livelock on queue processing

6ddfec4

fixes

39c0219

tavplubix added 3 commits June 17, 2022 13:47

fix style check, add comments

5ffc38d

Merge branch 'master' into fix_flaky_tests_with_transactions

e9c2157

fix race

83adf56

azat mentioned this pull request Jun 18, 2022

Randomize settings related to in-order read/aggregation #36914

Merged

3 tasks

Merge branch 'master' into fix_flaky_tests_with_transactions

4ed606a

Merge branch 'master' into fix_flaky_tests_with_transactions

9bdd9e1

tavplubix merged commit 75b8800 into master Jun 21, 2022

tavplubix deleted the fix_flaky_tests_with_transactions branch June 21, 2022 06:38

tavplubix mentioned this pull request Jul 8, 2022

Transaction has invalid state RUNNING #39028

Closed

azat reviewed Jul 8, 2022

View reviewed changes

tavplubix mentioned this pull request Nov 30, 2022

Deadlock in ReplicatedMergeTreeQueue/MergeTreeBackgroundExecutor when waiting for concurrently executing entry #43816

Closed

filimonov mentioned this pull request Dec 9, 2023

Slower replica: non-efficient execution of replication_queue (since 22.7) #57702

Closed

	/// It can be used in critical places to exit on unexpected exceptions.
	/// SIGABRT is usually better that broken in-memory state with unpredictable consequences.
	/// It also temporarily disables exception from memory tracker in current thread.
	/// Strict version does not take into account nested exception (i.e. it aborts even when we're in catch block).

Conversation

tavplubix commented Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Uh oh!

tavplubix commented Jun 3, 2022

Uh oh!

mergify bot commented Jun 3, 2022

✅ Branch has been successfully updated

Uh oh!

tavplubix commented Jun 17, 2022

Uh oh!

tavplubix commented Jun 20, 2022

Uh oh!

tavplubix commented Jun 20, 2022

Uh oh!

mergify bot commented Jun 20, 2022

✅ Branch has been successfully updated

Uh oh!

tavplubix commented Jun 20, 2022

Uh oh!

tavplubix commented Jun 20, 2022

Uh oh!

mergify bot commented Jun 20, 2022

✅ Branch has been successfully updated

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azat Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tavplubix commented Jun 3, 2022 •

edited

Loading

azat Jul 14, 2022 •

edited

Loading