#64: Speedb artifacts #66

isaac-io · 2022-07-25T21:18:43Z

This PR implements the initial phase of the Speedb artefacts differentiation (#64). That phase deals with renaming the output artefacts from RocksDB to Speedb, as well as replacing any references to RocksDB that are outputted by the code to Speedb.

This includes:

librocksdb.{a,so} -> libspeedb.{a,so} (and the SO name was changed from 7.2.2 to 1.5.0)
librocksdbjni*.{a,so} -> libspeedbjni*.{a,so} (the same SO name change was made here as well)
rocksdbjni*.jar -> speedbjni*.jar (here too, the JAR bears the Speedb version now)

Additionally, the following tools with RocksDB in their name have been renamed:

rocksdb_dump -> speedb_dump
rocksdb_undump -> speedb_undump

As part of the build changes, all build-time configuration variables were changed from ROCKSDB_* to SPEEDB_* (e.g. ROCKSDB_LITE is now SPEEDB_LITE. Note that only the build-time configuration was changed. The macro in the source code is still named ROCKSDB_LITE), and the CMake package name is now Speedb::speedb instead of RocksDB::rocksdb.

Version information that is printed by tools was changed to use the Speedb version information (except for trace_replay which uses that information as database version for format compatibility checks).

Additionally, a bunch of scripts were updated to point at the Speedb repository instead of the RocksDB one and to refer to the newly introduced artefact names.

For ease of review, the changes were broken into categories, each in its own commit, the largest of which is unfortunately the first one which deals with build-related changes.

Test Plan: Build all make and CMake targets and run make check and make jtest.

isaac-io · 2022-07-25T21:20:05Z

Note that due to the merge of #25, make check on main (which this PR's branch is based on) is currently broken (see #14 (comment)). However, with #25 reverted all of the tests pass successfully.

Update: not relevant anymore with the integration of #77.

udi-speedb · 2022-07-27T13:58:49Z

A basic question about the new names.
Rocksdb has either RocksDB or rocksdb.
Are we supposed to use Speedb or SpeeDB instead of RocksDB?
Are we supposed to use speedb intead of rocksdb?

CMakeLists.txt

isaac-io · 2022-07-27T14:16:49Z

A basic question about the new names. Rocksdb has either RocksDB or rocksdb. Are we supposed to use Speedb or SpeeDB instead of RocksDB? Are we supposed to use speedb intead of rocksdb?

The SpeeDB spelling should not be used according to marketing. So that leaves us with:

RocksDB -> Speedb
rocksdb-> speedb

CMakeLists.txt

udi-speedb · 2022-07-27T14:34:49Z

I see that the files used to create the speedb_dump / speedb_undump remain rocksdb_dump* / rocksdb_undump*. That seems a bit weird IMO.

udi-speedb · 2022-07-27T14:41:06Z

I see multiple references to rocksdb/version.h still in the repo. Is that on purpose:
.github/workflows/new_release_line.yml, .github/workflows/versioning.yml, db/db_impl/db_impl.cc, env/unique_id_gen.cc, include/rocksdb/db.h, include/rocksdb/options.h, java/rocksjni/rocksjni.cc, util/build_version.cc.in

isaac-io · 2022-07-27T15:35:54Z

I see that the files used to create the speedb_dump / speedb_undump remain rocksdb_dump* / rocksdb_undump*. That seems a bit weird IMO.

That's indeed weird, but the idea is to keep the code changes to a minimum (in this case it's only for avoiding pain in future rebases). Since we only care about the output artefacts, this is enough to fulfill this requirement.

I see multiple references to rocksdb/version.h still in the repo. Is that on purpose: .github/workflows/new_release_line.yml, .github/workflows/versioning.yml, db/db_impl/db_impl.cc, env/unique_id_gen.cc, include/rocksdb/db.h, include/rocksdb/options.h, java/rocksjni/rocksjni.cc, util/build_version.cc.in

.github/workflows/* -- References are mostly DevOps-related and need to be fixed separately.
db/db_impl/db_impl.cc -- This is simply the DumpRocksDBBuildVersion() function referencing the GetRocksBuildProperties() function which is declared in include/rocksdb/version.h and can't be renamed or removed due to backwards compatibility.
env/unique_id_gen.cc -- This is only used for setting the version_identifier field of the Entropy struct, which is hashed for unique ID generation. For backwards compatibility we probably want to keep using the RocksDB version here, and because it's never outputted as it anyway.
include/rocksdb/db.h -- Used for the kMajorVersion and kMinorVersion constants. This has to be kept for backward compatibility with RocksDB-generated I/O traces, as well as user code that might be depending on it (note that the Speedb version parts aren't actually exposed to user code anywhere in the public API, and that's by design, in order to keep compatibility at the code level).
include/rocksdb/options.h -- This doesn't actually use anything from include/rocksdb/version.h.
java/rocksjni/rocksjni.cc -- This uses the RocksDB version in the org.rocksdb.RocksDB.version() function that returns the version encoded as an integer. This might be depended upon by Java user code.
util/build_version.cc.in -- This is for getting the RocksDB version for user code the depends on it by calling the GetRocksVersionAsString() function.

isaac-io · 2022-07-27T15:39:22Z

I did find a place where the SpeeDB spelling was used and I forgot to update it, so I pushed a fix.

build_tools/check-sources.sh

build_tools/make_package.sh

cache/cache_bench_tool.cc

db/db_impl/db_impl.cc

db_stress_tool/db_stress_gflags.cc

mrambacher · 2022-07-27T15:27:38Z

build_tools/build_detect_platform

-echo "ROCKSDB_MAJOR=$ROCKSDB_MAJOR" >> "$OUTPUT"
-echo "ROCKSDB_MINOR=$ROCKSDB_MINOR" >> "$OUTPUT"
-echo "ROCKSDB_PATCH=$ROCKSDB_PATCH" >> "$OUTPUT"
+echo "SPEEDB_MAJOR=$SPEEDB_MAJOR" >> "$OUTPUT"
+echo "SPEEDB_MINOR=$SPEEDB_MINOR" >> "$OUTPUT"
+echo "SPEEDB_PATCH=$SPEEDB_PATCH" >> "$OUTPUT"


Does both speedb and rocksdb version information need to be present in the platform.mk file?

make_config.mk is only used by the Makefile, so there's no need for both.

CMakeLists.txt

Makefile

mrambacher

I think this change is more than is necessary and is going to be hard to maintain and test going forward. Changing the names of the variables and targets in the Makefile is not really necessary and is likely to be more pain that it is worth.

Many of these changes can be separated into a separate upstreamable PR (like allowing the names of libraries to change, adding a LITE mode status.

Makefile

tools/ldb_tool.cc

udi-speedb · 2022-07-28T06:21:34Z

@isaac-io - I am done. I have no further comments

mrambacher

I feel there are way more changes here than are necessary for branding. We should be focusing on only change the names of the artifacts (libraries, packages, etc) and not the names of all of the compiler flags.

isaac-io · 2022-08-16T19:08:16Z

Updated the PR to reduce the scope of the changes to the artefact names, logging, and version numbers only. Some of the work (setting the library name based on the project name, removing explicit mentions of RocksDB in status strings, etc.) can theoretically be upstreamable, if RocksDB would be open to accepting something like that.

tools/ldb_tool.cc

tools/db_bench_tool.cc

mrambacher · 2022-08-24T20:11:40Z

tools/db_bench_tool.cc

+    fprintf(stderr, "Speedb:    version %s\n",
+            GetSpeedbVersionAsString(false).c_str());



Why not just use the GetRocksBuildInfoAsString method?

Because it breaks the output formatting (alignment of the "version ..." string with the subsequently printed lines).

tools/benchmark.sh

mrambacher · 2022-08-24T20:23:59Z

fuzz/Makefile

+LDFLAGS += $(PLATFORM_LDFLAGS) $(LIB_FUZZING_ENGINE) $(PROTOBUF_MUTATOR_LDFLAGS) $(PROTOBUF_LDFLAGS) -L$(LIB_LIB_DIR) -l$(LIBNAME:lib%=%)
 endif


I do not think LIBNAME is specified here, is it?

It is, through the inclusion of make_config.mk. The fuzz Makefile is a weird creature. It requires manually building the library with fuzzing enabled and then cd'ing into the fuzz directory and building the fuzzing tools.

examples/Makefile

db_stress_tool/db_stress_gflags.cc

mrambacher · 2022-08-24T20:27:56Z

db/db_impl/db_impl.cc

@@ -4443,7 +4443,7 @@ void DBImpl::EraseThreadStatusDbInfo() const {}
 //
 // A global method that can dump out the build version
 void DumpRocksDBBuildVersion(Logger* log) {
-  ROCKS_LOG_HEADER(log, "SpeeDB version: %s (%s)\n",
+  ROCKS_LOG_HEADER(log, "Speedb version: %s (%s)\n",


Isn't there a method that gets/prints both already?

Not in this specific format. Changing the format will break tools the grep the log output to find the version (we have internal tools that do just that).

mrambacher · 2022-08-24T20:28:30Z

cache/cache_bench_tool.cc

+    printf("Speedb version     : %s\n",
+           GetSpeedbVersionAsString(false).c_str());


Use the print method.

The print method breaks the formatting of the output (alignment of the colon).

mrambacher · 2022-08-24T20:34:59Z

Makefile

+LIB_JAVA_VERSION ?= $(LIB_MAJOR).$(LIB_MINOR).$(LIB_PATCH)
+LIB_JAR = $(PROJECT_NAME)jni-$(LIB_JAVA_VERSION)-linux$(ARCH)$(JNI_LIBC_POSTFIX).jar
+LIB_JAR_ALL = $(PROJECT_NAME)jni-$(LIB_JAVA_VERSION).jar
+LIB_JAVADOCS_JAR = $(PROJECT_NAME)jni-$(LIB_JAVA_VERSION)-javadoc.jar
+LIB_SOURCES_JAR = $(PROJECT_NAME)jni-$(LIB_JAVA_VERSION)-sources.jar


I am not a huge fan of these LIB_xxx names. The LIB_ does not really convey much to me. Seems like we could come up with something better or just drop the lib part?

Dropping the LIB_ part is a bit difficult for naming these variables, especially for LIB_JAVA_VERSION where it would change the meaning completely. That prefix conveys the meaning that those variables are internal to the library. I can probably append NAME where applicable, but it wouldn't make the LIB_ prefix redundant.

mrambacher

I have not run any of the tests on this code. If it passes the C++ and Java test suite, good enough for me.

mrambacher · 2022-08-25T15:03:14Z

build_tools/build_detect_platform

+LIB_MAJOR=`build_tools/version.sh major`
+LIB_MINOR=`build_tools/version.sh minor`
+LIB_PATCH=`build_tools/version.sh patch`


Nit: I think these would be better named as "MAJOR_VERSION, MINOR_VERSION" etc. The "LIB" part seems off to me for some reason

I renamed them to VERSION_MAJOR, VERSION_MINOR, and VERSION_PATCH to mirror the existing SHARED_MAJOR etc. variables.

This updates the CMake and Makefile configuration and related scripts to use variables where possible and avoid explicit references to RocksDB in order to allow rebranding of the output artefacts.

This includes both the CMake and Makefile configurations, various scripts that are invoked during the build or during test runs, as well as the Java native library loader code.

This includes simple renaming from `rocksdb` to `speedb` as well as replacing URLs that are pointing to the RocksDB repository with the URL of the Speedb repository.

This includes references in statuses as well as tools output.

This is done across all tools except trace_replay, which specifically looks for the RocksDB version for database compatibility checks.

Both were incorrectly referencing the old artefact names before the changes in #66.

isaac-io requested a review from mrambacher July 25, 2022 21:18

isaac-io self-assigned this Jul 25, 2022

isaac-io linked an issue Jul 25, 2022 that may be closed by this pull request

Speedb Artifacts #64

Closed

isaac-io force-pushed the 64-speedb-artifacts branch from 73f7ff2 to f19fb7e Compare July 25, 2022 21:27

isaac-io requested review from udi-speedb and removed request for mrambacher July 26, 2022 12:28

udi-speedb reviewed Jul 27, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

udi-speedb reviewed Jul 27, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

isaac-io force-pushed the 64-speedb-artifacts branch from f19fb7e to 1f0c677 Compare July 27, 2022 15:38

isaac-io force-pushed the 64-speedb-artifacts branch from 1f0c677 to 5520d41 Compare July 27, 2022 15:42

mrambacher suggested changes Jul 27, 2022

View reviewed changes

mrambacher reviewed Jul 27, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

tools/ldb_tool.cc Outdated Show resolved Hide resolved

isaac-io force-pushed the 64-speedb-artifacts branch 3 times, most recently from 388c951 to fdd806d Compare July 28, 2022 05:57

isaac-io force-pushed the 64-speedb-artifacts branch from fdd806d to f87412a Compare July 28, 2022 07:05

mrambacher suggested changes Jul 31, 2022

View reviewed changes

isaac-io force-pushed the 64-speedb-artifacts branch from f87412a to 3b0da0b Compare August 16, 2022 19:04

isaac-io force-pushed the 64-speedb-artifacts branch from 3b0da0b to 2b3b558 Compare August 16, 2022 19:14

isaac-io force-pushed the 64-speedb-artifacts branch from 2b3b558 to 08f1f93 Compare August 24, 2022 10:37

mrambacher reviewed Aug 24, 2022

View reviewed changes

isaac-io force-pushed the 64-speedb-artifacts branch from 08f1f93 to 1123fd5 Compare August 25, 2022 10:54

mrambacher approved these changes Aug 25, 2022

View reviewed changes

isaac-io force-pushed the 64-speedb-artifacts branch 2 times, most recently from 46eb01d to 2ed6e6f Compare August 25, 2022 16:24

isaac-io force-pushed the 64-speedb-artifacts branch 2 times, most recently from c288740 to 2b4a6c0 Compare September 21, 2022 13:33

isaac-io force-pushed the 64-speedb-artifacts branch from 2b4a6c0 to ff6bb48 Compare October 3, 2022 12:26

mrambacher approved these changes Oct 3, 2022

View reviewed changes

isaac-io force-pushed the 64-speedb-artifacts branch from ff6bb48 to b5f692e Compare October 20, 2022 08:02

isaac-io added 5 commits October 24, 2022 22:50

build: clean up the build configuration to allow rebranding (#64)

23a28bf

This updates the CMake and Makefile configuration and related scripts to use variables where possible and avoid explicit references to RocksDB in order to allow rebranding of the output artefacts.

build: rename artefacts from RocksDB to Speedb (#64)

b7b3da8

This includes both the CMake and Makefile configurations, various scripts that are invoked during the build or during test runs, as well as the Java native library loader code.

scripts: rename RocksDB references to Speedb (#64)

e23c78b

This includes simple renaming from `rocksdb` to `speedb` as well as replacing URLs that are pointing to the RocksDB repository with the URL of the Speedb repository.

general: replace RocksDB references in strings with Speedb (#64)

871738b

This includes references in statuses as well as tools output.

tools: replace RocksDB version information with the Speedb version (#64)

456a5bc

This is done across all tools except trace_replay, which specifically looks for the RocksDB version for database compatibility checks.

isaac-io force-pushed the 64-speedb-artifacts branch from b5f692e to 456a5bc Compare October 25, 2022 14:06

isaac-io merged commit 1169c43 into main Oct 25, 2022

isaac-io deleted the 64-speedb-artifacts branch October 25, 2022 14:39

isaac-io added a commit that referenced this pull request Nov 10, 2022

docs: fix instructions for building Speedb in README.md and INSTALL.md

f0a8068

Both were incorrectly referencing the old artefact names before the changes in #66.

isaac-io mentioned this pull request Nov 10, 2022

docs: fix instructions for building Speedb in README.md and INSTALL.md #229

Merged

isaac-io added a commit that referenced this pull request Nov 10, 2022

docs: fix instructions for building Speedb in README.md and INSTALL.md

4c14891

Both were incorrectly referencing the old artefact names before the changes in #66.

Yuval-Ariel pushed a commit that referenced this pull request Nov 15, 2022

docs: fix instructions for building Speedb in README.md and INSTALL.md

c667797

Both were incorrectly referencing the old artefact names before the changes in #66.

Yuval-Ariel pushed a commit that referenced this pull request Nov 25, 2022

docs: fix instructions for building Speedb in README.md and INSTALL.md

77e3dc8

Both were incorrectly referencing the old artefact names before the changes in #66.

Yuval-Ariel pushed a commit that referenced this pull request Nov 25, 2022

docs: fix instructions for building Speedb in README.md and INSTALL.md

cf2fbe3

Both were incorrectly referencing the old artefact names before the changes in #66.

Yuval-Ariel pushed a commit that referenced this pull request Apr 30, 2023

docs: fix instructions for building Speedb in README.md and INSTALL.md

e34c6a2

Both were incorrectly referencing the old artefact names before the changes in #66.

Yuval-Ariel pushed a commit that referenced this pull request May 4, 2023

docs: fix instructions for building Speedb in README.md and INSTALL.md

cc59224

Both were incorrectly referencing the old artefact names before the changes in #66.

udi-speedb pushed a commit that referenced this pull request Nov 13, 2023

docs: fix instructions for building Speedb in README.md and INSTALL.md

1074454

Both were incorrectly referencing the old artefact names before the changes in #66.

udi-speedb pushed a commit that referenced this pull request Nov 15, 2023

docs: fix instructions for building Speedb in README.md and INSTALL.md

0ca3613

Both were incorrectly referencing the old artefact names before the changes in #66.

udi-speedb pushed a commit that referenced this pull request Dec 3, 2023

docs: fix instructions for building Speedb in README.md and INSTALL.md

d5351a1

Both were incorrectly referencing the old artefact names before the changes in #66.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#64: Speedb artifacts #66

#64: Speedb artifacts #66

isaac-io commented Jul 25, 2022 •

edited

Loading

isaac-io commented Jul 25, 2022 •

edited

Loading

udi-speedb commented Jul 27, 2022

isaac-io commented Jul 27, 2022

udi-speedb commented Jul 27, 2022

udi-speedb commented Jul 27, 2022 •

edited

Loading

isaac-io commented Jul 27, 2022 •

edited

Loading

isaac-io commented Jul 27, 2022 •

edited

Loading

mrambacher Jul 27, 2022

isaac-io Jul 27, 2022

mrambacher left a comment

udi-speedb commented Jul 28, 2022

mrambacher left a comment

isaac-io commented Aug 16, 2022 •

edited

Loading

mrambacher Aug 24, 2022

isaac-io Aug 25, 2022

mrambacher Aug 24, 2022

isaac-io Aug 25, 2022

mrambacher Aug 24, 2022

isaac-io Aug 25, 2022

mrambacher Aug 24, 2022

isaac-io Aug 25, 2022

mrambacher Aug 24, 2022

isaac-io Aug 25, 2022

mrambacher left a comment

mrambacher Aug 25, 2022

isaac-io Aug 25, 2022

		fprintf(stderr, "Speedb: version %s\n",
		GetSpeedbVersionAsString(false).c_str());

		LDFLAGS += $(PLATFORM_LDFLAGS) $(LIB_FUZZING_ENGINE) $(PROTOBUF_MUTATOR_LDFLAGS) $(PROTOBUF_LDFLAGS) -L$(LIB_LIB_DIR) -l$(LIBNAME:lib%=%)
		endif

		printf("Speedb version : %s\n",
		GetSpeedbVersionAsString(false).c_str());

#64: Speedb artifacts #66

#64: Speedb artifacts #66

Conversation

isaac-io commented Jul 25, 2022 • edited Loading

isaac-io commented Jul 25, 2022 • edited Loading

udi-speedb commented Jul 27, 2022

isaac-io commented Jul 27, 2022

udi-speedb commented Jul 27, 2022

udi-speedb commented Jul 27, 2022 • edited Loading

isaac-io commented Jul 27, 2022 • edited Loading

isaac-io commented Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrambacher left a comment

Choose a reason for hiding this comment

udi-speedb commented Jul 28, 2022

mrambacher left a comment

Choose a reason for hiding this comment

isaac-io commented Aug 16, 2022 • edited Loading

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

mrambacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaac-io commented Jul 25, 2022 •

edited

Loading

isaac-io commented Jul 25, 2022 •

edited

Loading

udi-speedb commented Jul 27, 2022 •

edited

Loading

isaac-io commented Jul 27, 2022 •

edited

Loading

isaac-io commented Jul 27, 2022 •

edited

Loading

isaac-io commented Aug 16, 2022 •

edited

Loading