Skip to content

Upgrade rocksdb version to v9.2.2#67274

Closed
canhld94 wants to merge 10 commits intoClickHouse:masterfrom
canhld94:upstream_upgrade_rocksdb
Closed

Upgrade rocksdb version to v9.2.2#67274
canhld94 wants to merge 10 commits intoClickHouse:masterfrom
canhld94:upstream_upgrade_rocksdb

Conversation

@canhld94
Copy link
Copy Markdown
Contributor

@canhld94 canhld94 commented Jul 27, 2024

Changelog category (leave one):

  • Improvement

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

  • Upgrade rocksdb library version to v9.2.2

Main motivation is because current rocksdb version is too old. Once we have some SST files built by default bookworm rocksdb (8.10 iirc) and it cannot be read by ClickHouse rocksdb engine.

In addition, as discussed in #66479, upgrading from current version -> 9.2.2 will introduce 2 breaking changes:

  1. In version 6.27.0 new hash type kXXH3 was added. Then in v7.8.0 kXXH3 was made default.
  2. Same, current version format_version is 5, but from in version 8.x format_version = 6 was introduced and made by default.

So we try to keep the new version data compatible with current version data (just in case anyone need to rollback) by explicitly set:

table_options.checksum = rocksdb::ChecksumType::kxxHash64;
table_options.format_version = 5;

When the new version is stable, we can remove above code and use default value in rocksdb 9.2.2.

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

CI Settings (Only check the boxes if you know what you are doing):

  • Allow: All Required Checks
  • Allow: Stateless tests
  • Allow: Stateful tests
  • Allow: Integration Tests
  • Allow: Performance tests
  • Allow: All Builds
  • Allow: batch 1, 2 for multi-batch jobs
  • Allow: batch 3, 4, 5, 6 for multi-batch jobs
  • Exclude: Style check
  • Exclude: Fast test
  • Exclude: All with ASAN
  • Exclude: All with TSAN, MSAN, UBSAN, Coverage
  • Exclude: All with aarch64, release, debug
  • Do not test
  • Woolen Wolfdog
  • Upload binaries for special builds
  • Disable merge-commit
  • Disable CI cache

canhld94 added 3 commits July 27, 2024 09:06
@canhld94
upgrade rocksdb version to 9.2.2
5c38107 
Signed-off-by: Duc Canh Le <duccanh.le@ahrefs.com>
@canhld94
fix for backward incompatibility and performance regression
547a63b 
Signed-off-by: Duc Canh Le <duccanh.le@ahrefs.com>
@canhld94
fix build and add a compatibility test
f5aa92a 
Signed-off-by: Duc Canh Le <duccanh.le@ahrefs.com>
@canhld94
Copy link
Copy Markdown
Contributor Author

canhld94 commented Jul 27, 2024

cc @rschu1ze
Currently I'm using rocksdb repo at https://github.com/canhld94/rocksdb/tree/clickhouse/v9.2.2 (v9.2.2 + some extra commits from ClickHouse repo). Can you help to move it to https://github.com/ClickHouse/rocksdb [THANKS].

canhld94 added 2 commits July 27, 2024 10:24
@canhld94
update format_version to 5
f5fc671 
Signed-off-by: Duc Canh Le <duccanh.le@ahrefs.com>
@canhld94
update comments
22b8e6e 
Signed-off-by: Duc Canh Le <duccanh.le@ahrefs.com>
@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Jul 27, 2024
@robot-ch-test-poll4 robot-ch-test-poll4 added pr-improvement Pull request with some product improvements submodule changed At least one submodule changed in this PR. labels Jul 27, 2024
@robot-clickhouse-ci-1
Copy link
Copy Markdown
Contributor

robot-clickhouse-ci-1 commented Jul 27, 2024 •
edited by robot-ch-test-poll4
Loading

This is an automated comment for commit ac48f11 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check nameDescriptionStatus
Performance ComparisonMeasure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests❌ failure
Stateless testsRuns stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc❌ failure
Successful checks
Check nameDescriptionStatus
AST fuzzerRuns randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help✅ success
BuildsThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS✅ success
ClickBenchRuns [ClickBench](https://github.com/ClickHouse/ClickBench/) with instant-attach table✅ success
Compatibility checkChecks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help✅ success
Docker keeper imageThe check to build and optionally push the mentioned image to docker hub✅ success
Docker server imageThe check to build and optionally push the mentioned image to docker hub✅ success
Docs checkBuilds and tests the documentation✅ success
Fast testNormally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here✅ success
Flaky testsChecks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integration tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc✅ success
Install packagesChecks that the built packages are installable in a clear environment✅ success
Integration testsThe integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests✅ success
Stateful testsRuns stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc✅ success
Stress testRuns stateless functional tests concurrently from several clients to detect concurrency-related errors✅ success
Style checkRuns a set of checks to keep the code style clean. If some of tests failed, see the related log from the report✅ success
Unit testsRuns the unit tests for different release types✅ success
Upgrade checkRuns stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts✅ success

canhld94 added 3 commits July 28, 2024 09:17
@canhld94
not manually adding compiler specific flags
1b877a5 
Signed-off-by: Duc Canh Le <duccanh.le@ahrefs.com>
@canhld94
remove an useless line
f84e2d2 
Signed-off-by: Duc Canh Le <duccanh.le@ahrefs.com>
@canhld94
cmake fix
516fdec 
Signed-off-by: Duc Canh Le <duccanh.le@ahrefs.com>
@canhld94
Copy link
Copy Markdown
Contributor Author

canhld94 commented Jul 29, 2024 •
edited
Loading

Re. failing test in https://s3.amazonaws.com/clickhouse-test-reports/67274/f84e2d29482879aeedb70af9d89f1014df787c6d/stateless_tests__asan__[1_2].html:

I don't understand yet why we explicitly set rocksdb log level to error, but the warning messages still being printed:

2024-07-28 08:08:51 Reason: having stderror:  
2024-07-28 08:08:51 [9d89e5687183] 2024.07.28 05:08:48.177662 [ 51097 ] {131ea5f5-19c6-433c-b959-1a40c4e674e1} <Warning> StorageEmbeddedRocksDB (test_6uvtfta1.dict_with_ttl (bc57c65f-fb09-4e93-9b5c-bbe1d2e7e371)): [table/block_based/block_based_table_reader.cc:863] Tail prefetch size 524288 is calculated based on heuristics
2024-07-28 08:08:51 
2024-07-28 08:08:51 stdout:
2024-07-28 08:08:51 0	foo
2024-07-28 08:08:51 0

May be @azat do you have any idea?

@canhld94
Copy link
Copy Markdown
Contributor Author

canhld94 commented Jul 29, 2024 •
edited
Loading

Re. failing test in https://s3.amazonaws.com/clickhouse-test-reports/67274/f84e2d29482879aeedb70af9d89f1014df787c6d/stateless_tests__asan__[1_2].html:

I don't understand yet why we explicitly set rocksdb log level to error, but the warning messages still being printed:

2024-07-28 08:08:51 Reason: having stderror:  
2024-07-28 08:08:51 [9d89e5687183] 2024.07.28 05:08:48.177662 [ 51097 ] {131ea5f5-19c6-433c-b959-1a40c4e674e1} <Warning> StorageEmbeddedRocksDB (test_6uvtfta1.dict_with_ttl (bc57c65f-fb09-4e93-9b5c-bbe1d2e7e371)): [table/block_based/block_based_table_reader.cc:863] Tail prefetch size 524288 is calculated based on heuristics
2024-07-28 08:08:51 
2024-07-28 08:08:51 stdout:
2024-07-28 08:08:51 0	foo
2024-07-28 08:08:51 0

May be @azat do you have any idea?

So it turns out that in CI we set rocksdb log level to debug to trace some unstable test.
The unstable test was removed anyway.
Regarding some warning message during importing data, it all relates to construction of a new table reader during ingestion of external SST file in external_sst_file_ingestion_job.cc

  status = cfd_->ioptions()->table_factory->NewTableReader(
      TableReaderOptions(
          *cfd_->ioptions(), sv->mutable_cf_options.prefix_extractor,
          env_options_, cfd_->internal_comparator(),
          sv->mutable_cf_options.block_protection_bytes_per_key,
          /*skip_filters*/ false, /*immortal*/ false,
          /*force_direct_prefetch*/ false, /*level*/ -1,
          /*block_cache_tracer*/ nullptr,
          /*max_file_size_for_l0_meta_pin*/ 0, versions_->DbSessionId(),
          /*cur_file_num*/ new_file_number,
          /* unique_id */ {}, /* largest_seqno */ 0,
          /* tail_size */ 0, user_defined_timestamps_persisted),
      std::move(sst_file_reader), file_to_ingest->file_size, table_reader);
  return status;

Here unique_id = {} and thus when opening DB we have warning At least one SST file opened without unique ID to verify... and tail_size = 0, so we have warning Tail prefetch size xxx is calculated based on heuristics. It's more likely a message at debug level to me, not warning.

So proposed action is to change log level in CI to error.

Note: Looking at this facebook/rocksdb#12667 (review), Tail prefetch size xxx is calculated based on heuristics is at warning level because rocksdb maintainer want to track use cases that are at risk of regression when finally remove TailPrefetchStats.

@canhld94
change rocksdb log level in CI to ERROR_LEVEL
cfdb7cb 
Signed-off-by: Duc Canh Le <duccanh.le@ahrefs.com>
@rschu1ze rschu1ze self-assigned this Jul 29, 2024
@canhld94
Copy link
Copy Markdown
Contributor Author

canhld94 commented Jul 30, 2024 •
edited
Loading

No sanitizer error found.. Not a surprise because we also have sanitizer build tests in CI and had been running this rocksdb version for a month (though less comprehensive than ClickHouse CI).

Flaky test because I forget to drop table after test. Let's run CI for few more iteration.

@canhld94
drop table in test
ac48f11 
Signed-off-by: Duc Canh Le <duccanh.le@ahrefs.com>
@rschu1ze rschu1ze mentioned this pull request Jul 31, 2024
Merged
@canhld94
Copy link
Copy Markdown
Contributor Author

canhld94 commented Aug 1, 2024

To be done in #66479

@canhld94 canhld94 closed this Aug 1, 2024
@rschu1ze rschu1ze mentioned this pull request Aug 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@rschu1ze rschu1ze

Labels

can be tested Allows running workflows for external contributors pr-improvement Pull request with some product improvements submodule changed At least one submodule changed in this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@canhld94 @robot-clickhouse-ci-1 @alexey-milovidov @robot-ch-test-poll4 @rschu1ze