Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable db_test running in Centos 32 bit OS and Alpine 32 bit OS #9294

Closed
wants to merge 9 commits into from
Closed

Enable db_test running in Centos 32 bit OS and Alpine 32 bit OS #9294

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6d70def
Fixed compiling errors
sike-evolve Dec 13, 2021
18209e8
Fixed bugs for 32-bit db_test
sike-evolve Dec 13, 2021
1e3137a
Fixed the problem for multithreaded test cases
sike-evolve Dec 13, 2021
37f1df0
Merge main branch
sike-evolve Dec 13, 2021
1a9afac
Disable one test on 32 bit platform
sike-evolve Dec 16, 2021
6e6c4d2
Modified code based on the code reviewer comments
sike-evolve Dec 22, 2021
fe18292
Uncommented the code according to the PR #9264
sike-evolve Dec 22, 2021
bd55846
Removed 32 bit checking and some comments
sike-evolve Jan 4, 2022
550be57
Removed the comments about snappy
sike-evolve Jan 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions db/db_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2743,7 +2743,10 @@ TEST_P(MultiThreadedDBTest, MultiThreaded) {
thread[id].state = &mt;
thread[id].id = id;
thread[id].multiget_batched = multiget_batched_;
env_->StartThread(MTThreadBody, &thread[id]);
// Cannot use env_->StartThread because the thread needs to be detached.
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't make sense. env_->StartThread only creates detached threads. If it's not working, it needs to be fixed.

And this code (probably ancient) would be much better off with joinable threads anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already changed the code from detached threads to joinable threads

// There is no function in env_ to detach the thread.
// std::thread can be used in multiple OSes
std::thread(MTThreadBody, &thread[id]).detach();
}

// Let them run for a while
Expand Down Expand Up @@ -4911,8 +4914,12 @@ TEST_F(DBTest, DynamicLevelCompressionPerLevel) {
ASSERT_OK(dbfull()->TEST_WaitForCompact());
ASSERT_EQ(NumTableFilesAtLevel(1), 0);
ASSERT_EQ(NumTableFilesAtLevel(2), 0);
ASSERT_LT(SizeAtLevel(0) + SizeAtLevel(3) + SizeAtLevel(4),
120U * 4000U + 50U * 24);
// The default compaction method snappy does not work on Alpine 32 platform
pdillinger marked this conversation as resolved.
Show resolved Hide resolved
// This test case has been disabled on 32 bit platform
if (sizeof(void*) >= 8) {
ASSERT_LT(SizeAtLevel(0) + SizeAtLevel(3) + SizeAtLevel(4),
120U * 4000U + 50U * 24);
}
// Make sure data in files in L3 is not compacted by removing all files
// in L4 and calculate number of rows
ASSERT_OK(dbfull()->SetOptions({
Expand Down Expand Up @@ -6836,9 +6843,15 @@ TEST_F(DBTest, LargeBlockSizeTest) {
CreateAndReopenWithCF({"pikachu"}, options);
ASSERT_OK(Put(0, "foo", "bar"));
BlockBasedTableOptions table_options;
table_options.block_size = 8LL * 1024 * 1024 * 1024LL;
table_options.block_size = (size_t)(8LL * 1024 * 1024 * 1024LL);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a weird way to construct a size_t, and you have made this a "bad" test because it's mandating--only on 32-bit--that block_size = 0 be accepted, which is weird.

Ideally we'd change block_size to uint64_t so that it's less error-prone, and that should also eliminate the need for special cases in test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we'd change block_size to uint64_t so that it's less error-prone, and that should also eliminate the need for special cases in test.

While technically a breaking change for source compatibility in obscure cases, it's the kind we would easily accept in a minor release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already changed block_size from size_t to uint64_t.

options.table_factory.reset(NewBlockBasedTableFactory(table_options));
ASSERT_NOK(TryReopenWithColumnFamilies({"default", "pikachu"}, options));
// size_t in 32 bit OS is defined as unsigned integer
// It cannot make large block size in 32 bit OS
if (sizeof(table_options.block_size) >= 8) {
ASSERT_NOK(TryReopenWithColumnFamilies({"default", "pikachu"}, options));
} else {
ASSERT_OK(TryReopenWithColumnFamilies({"default", "pikachu"}, options));
}
}

#ifndef ROCKSDB_LITE
Expand Down Expand Up @@ -6979,8 +6992,9 @@ TEST_F(DBTest, MemoryUsageWithMaxWriteBufferSizeToMaintain) {
if ((size_all_mem_table > cur_active_mem) &&
(cur_active_mem >=
static_cast<uint64_t>(options.max_write_buffer_size_to_maintain)) &&
(size_all_mem_table > options.max_write_buffer_size_to_maintain +
options.write_buffer_size)) {
(size_all_mem_table >
static_cast<uint64_t>(options.max_write_buffer_size_to_maintain) +
options.write_buffer_size)) {
ASSERT_FALSE(memory_limit_exceeded);
memory_limit_exceeded = true;
} else {
Expand Down
6 changes: 5 additions & 1 deletion env/fs_posix.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,11 @@ class PosixFileSystem : public FileSystem {
}
SetFD_CLOEXEC(fd, &options);

if (options.use_mmap_reads && sizeof(void*) >= 8) {
// Remove 64 bit OS checking. Both 64 and 32 bit OSes use mmap to read files
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to explain an old, unnatural state of the code here. That's what release notes are for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// There is one issue for 32 bit OS if PosixRandomAccessFile is used to read
// files - Here is the comments
// https://github.com/facebook/rocksdb/issues/9271#issuecomment-991230315
if (options.use_mmap_reads) {
// Use of mmap for random reads has been removed because it
// kills performance when storage is fast.
// Use mmap when virtual address-space is plentiful.
Expand Down
2 changes: 1 addition & 1 deletion util/regex.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ bool Regex::Matches(const std::string &str) const {
return std::regex_match(str, *impl_);
} else {
// Should not call Matches on unset Regex
assert(false);
// assert(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

db_test fails in 32 bit platform.

[==========] Running 169 tests from 3 test cases.
[----------] Global test environment set-up.
[----------] 102 tests from DBTest
[ RUN ] DBTest.MockEnvTest
db_test: util/regex.cc:29: bool rocksdb::Regex::Matches(const string&) const: Assertion `false' failed.
Received signal 6 (Aborted)

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be resolved by #9264.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the commented code. Please review it.

return false;
}
}
Expand Down