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

sike-evolve · 2021-12-13T22:01:45Z

adamretter · 2021-12-16T22:00:05Z

Just to add that we have also prepared CircleCI jobs for testing this here - #7876

pdillinger · 2021-12-20T19:29:08Z

db/db_test.cc

@@ -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.


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.

Already changed the code from detached threads to joinable threads

pdillinger · 2021-12-20T19:38:36Z

db/db_test.cc

@@ -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);


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.

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.

Already changed block_size from size_t to uint64_t.

pdillinger · 2021-12-20T19:40:41Z

util/regex.cc

@@ -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);


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)

This should be resolved by #9264.

Removed the commented code. Please review it.

pdillinger · 2021-12-20T19:42:31Z

env/fs_posix.cc

@@ -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


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

db/db_test.cc

pdillinger

Thanks for the work!

pdillinger · 2021-12-23T23:22:07Z

db/db_test.cc

 static void MTThreadBody(void* arg) {
  MTThread* t = reinterpret_cast<MTThread*>(arg);
  int id = t->id;
  DB* db = t->state->test->db_;
  int counter = 0;
+  auto start = std::chrono::high_resolution_clock::now();


How about

std::shared_ptr<SystemClock> clock = SystemClock::Default(); auto end_micros = clock->NowMicros() + kTestSeconds * 1000000U; ... while (clock->NowMicros() < end_micros) {

(And change kTestSeconds to unsigned)

pdillinger · 2021-12-23T23:23:23Z

db/db_test.cc

@@ -2587,15 +2585,23 @@ struct MTThread {
  bool multiget_batched;
 };

+// Based on Peter Dillinger's suggestion, the threads have been changed from


Not necessary. Thanks. Someone can use git blame if they want to see the history of this test.

pdillinger · 2021-12-23T23:26:21Z

db/db_test.cc

-      env_->SleepForMicroseconds(100000);
-    }
-  }
+  // Based on Peter Dillinger's suggestion, the threads have been changed from


Comment not necessary. Thanks

db/db_test.cc

adamretter · 2022-01-13T16:46:04Z

@pdillinger Now we have reconciled the Snappy issue to an older version of Snappy shipped on older versions of Alpine, are you happy with this PR?

pdillinger · 2022-01-14T00:59:42Z

db/db_test.cc

+
+  // The default compaction method snappy does not work on Alpine 32 platform
+  // That means there is no compression
+  // The following test case has been disabled on 32 bit platform


Isn't this comment now obsolete?

Now I removed this comment.

facebook-github-bot · 2022-01-14T18:02:47Z

@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

pdillinger

Awesome, thanks!

Summary: Loose ends relate to mmap on 32-bit systems. (Testing is more complicated when the feature is completely disabled on 32-bit.) Test Plan: CI

Summary: Loose ends relate to mmap on 32-bit systems. (Testing is more complicated when the feature was completely disabled on 32-bit.) Pull Request resolved: #9386 Test Plan: CI Reviewed By: ajkr Differential Revision: D33590715 Pulled By: pdillinger fbshipit-source-id: f2637036a538a552200adee65b6765fce8cae27b

sike-evolve added 4 commits December 13, 2021 17:44

Fixed compiling errors

6d70def

Fixed bugs for 32-bit db_test

18209e8

Fixed the problem for multithreaded test cases

1e3137a

Merge main branch

37f1df0

facebook-github-bot added the CLA Signed label Dec 13, 2021

Disable one test on 32 bit platform

1a9afac

adamretter requested a review from pdillinger December 20, 2021 16:10

sike-evolve marked this pull request as ready for review December 20, 2021 16:13

pdillinger reviewed Dec 20, 2021

View reviewed changes

sike-evolve added 2 commits December 22, 2021 05:42

Modified code based on the code reviewer comments

6e6c4d2

Uncommented the code according to the PR facebook#9264

fe18292

sike-evolve changed the title ~~Enable db_test running in Centos 32 bit OS~~ Enable db_test running in Centos 32 bit OS and Alpine 32 bit OS Dec 22, 2021

pdillinger reviewed Dec 23, 2021

View reviewed changes

adamretter mentioned this pull request Dec 24, 2021

RocksDB 6.22.1 fails to build on ARM #8609

Open

riversand963 mentioned this pull request Dec 30, 2021

Installation fails on Alpine Linux #6397

Closed

Removed 32 bit checking and some comments

bd55846

pdillinger reviewed Jan 14, 2022

View reviewed changes

Removed the comments about snappy

550be57

pdillinger approved these changes Jan 14, 2022

View reviewed changes

facebook-github-bot closed this in 93b1de4 Jan 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sike-evolve commented Dec 13, 2021 •

edited by adamretter

Loading

adamretter commented Dec 16, 2021

pdillinger Dec 20, 2021

sike-evolve Dec 23, 2021

pdillinger Dec 20, 2021

pdillinger Dec 20, 2021

sike-evolve Dec 23, 2021

pdillinger Dec 20, 2021

sike-evolve Dec 21, 2021

mrambacher Dec 21, 2021

sike-evolve Jan 13, 2022

pdillinger Dec 20, 2021

sike-evolve Jan 4, 2022

pdillinger left a comment

pdillinger Dec 23, 2021

sike-evolve Jan 4, 2022

pdillinger Dec 23, 2021

sike-evolve Jan 4, 2022

pdillinger Dec 23, 2021

sike-evolve Jan 4, 2022

adamretter commented Jan 13, 2022

pdillinger Jan 14, 2022

sike-evolve Jan 14, 2022

facebook-github-bot commented Jan 14, 2022

pdillinger left a comment

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

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

Conversation

sike-evolve commented Dec 13, 2021 • edited by adamretter Loading

adamretter commented Dec 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdillinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamretter commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 14, 2022

pdillinger left a comment

Choose a reason for hiding this comment

sike-evolve commented Dec 13, 2021 •

edited by adamretter

Loading