Skip to content

Test log level for CI#28559

Merged
alesapin merged 5 commits intomasterfrom
add_test_logs_level
Sep 6, 2021
Merged

Test log level for CI#28559
alesapin merged 5 commits intomasterfrom
add_test_logs_level

Conversation

@alesapin
Copy link
Copy Markdown
Member

@alesapin alesapin commented Sep 3, 2021

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Build/Testing/Packaging Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add new log level <test> for testing environments.

@robot-clickhouse robot-clickhouse added the pr-build Pull request with build/testing/packaging improvement label Sep 3, 2021
@robot-ch-test-poll3 robot-ch-test-poll3 added the submodule changed At least one submodule changed in this PR. label Sep 3, 2021
}
else
{
LOG_TEST(log, "Commit request for session {} with type {}, log id {}{}",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Just add one message for test.

} while (false)


#define LOG_TEST(logger, ...) LOG_IMPL(logger, DB::LogsLevel::test, Poco::Message::PRIO_TEST, __VA_ARGS__)
Copy link
Copy Markdown
Member

@azat azat Sep 3, 2021

Choose a reason for hiding this comment

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

Interesting idea, maybe it should be under #ifdef so that only non-production binaries on CI (i.e. everything with relasetype != relwithdebinfo) will enable those messages?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We use our production binary in all release tests (and it looks reasonable). I'll think about defence here, maybe at least some runtime check (env variable)...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm suggesting #ifdef here to make then literally no-op in relwithdebinfo/release builds (so that one will not care about additional cost for log messages, i.e. calling of some heavy method), but this has one downside - "variable maybe unused" warnings...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@azat It costs only one if.
Macro arguments are lazy evaluated.

@alesapin
Copy link
Copy Markdown
Member Author

alesapin commented Sep 6, 2021

Fuzzer:

SELECT
    topKResampleState(1048576, 65537, 1048577, 2)(toString(number), number),
    topKResampleState(3, 257, 9223372036854775807)(toString(number), number)
FROM numbers(1048577)

AddressSanitizer: allocator is out of memory trying to allocate 0x20000000 bytes

./obj-x86_64-linux-gnu/../src/Common/AllocatorWithMemoryTracking.h:35
./obj-x86_64-linux-gnu/../contrib/libcxx/include/__memory/allocator_traits.h:468
./obj-x86_64-linux-gnu/../contrib/libcxx/include/__split_buffer:314
./obj-x86_64-linux-gnu/../contrib/libcxx/include/vector:1624
./obj-x86_64-linux-gnu/../contrib/libcxx/include/vector:1641
./obj-x86_64-linux-gnu/../src/Common/SpaceSaving.h:310
./obj-x86_64-linux-gnu/../src/AggregateFunctions/AggregateFunctionTopK.h:176
./obj-x86_64-linux-gnu/../src/AggregateFunctions/AggregateFunctionResample.h:181
./obj-x86_64-linux-gnu/../src/DataTypes/Serializations/SerializationAggregateFunction.cpp:100
./obj-x86_64-linux-gnu/../src/DataTypes/Serializations/ISerialization.cpp:101
./obj-x86_64-linux-gnu/../src/DataStreams/NativeBlockInputStream.cpp:86
./obj-x86_64-linux-gnu/../src/DataStreams/NativeBlockInputStream.cpp:168
./obj-x86_64-linux-gnu/../src/DataStreams/IBlockInputStream.cpp:57
./obj-x86_64-linux-gnu/../src/Client/Connection.cpp:876
./obj-x86_64-linux-gnu/../src/Client/Connection.cpp:857
./obj-x86_64-linux-gnu/../src/Client/Connection.cpp:799
./obj-x86_64-linux-gnu/../programs/client/Client.cpp:2146
./obj-x86_64-linux-gnu/../programs/client/Client.cpp:2132
./obj-x86_64-linux-gnu/../programs/client/Client.cpp:1821
./obj-x86_64-linux-gnu/../programs/client/Client.cpp:1730
./obj-x86_64-linux-gnu/../programs/client/Client.cpp:1454
./obj-x86_64-linux-gnu/../programs/client/Client.cpp:1195
./obj-x86_64-linux-gnu/../programs/client/Client.cpp:931
./obj-x86_64-linux-gnu/../programs/client/Client.cpp:940
./obj-x86_64-linux-gnu/../programs/client/Client.cpp:745
./obj-x86_64-linux-gnu/../programs/client/Client.cpp:317
./obj-x86_64-linux-gnu/../contrib/poco/Util/src/Application.cpp:334
./obj-x86_64-linux-gnu/../programs/client/Client.cpp:2852
./obj-x86_64-linux-gnu/../programs/main.cpp:372

But now we have MemoryTracker in client, so we can set limit for it, so it should be Ok?

@alesapin alesapin merged commit 7b2d4a3 into master Sep 6, 2021
@alesapin alesapin deleted the add_test_logs_level branch September 6, 2021 07:50
@tavplubix tavplubix mentioned this pull request Oct 8, 2021
2 tasks
the-horhe pushed a commit to the-horhe/clickhouse-driver that referenced this pull request Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-build Pull request with build/testing/packaging improvement submodule changed At least one submodule changed in this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants