The ObjectRegistry class replaces the Registrar and NewCustomObjects.… #5293

mrambacher · 2019-05-09T18:22:50Z

The ObjectRegistry class replaces the Registrar and NewCustomObjects. Objects are registered with the registry by Type (the class must implement the static const char *Type() method).

This change is necessary for a few reasons:

By having a class (rather than static template instances), the class can be passed between compilation units, meaning that objects could be registered and shared from a dynamic library with an executable.
By having a class with instances, different units could have different objects registered. This could be useful if, for example, one Option allowed for a dynamic library and one did not.

When combined with some other PRs (being able to load shared libraries, a Configurable interface to configure objects to/from string), this code will allow objects in external shared libraries to be added to a RocksDB image at run-time, rather than requiring every new extension to be built into the main library and called explicitly by every program.

Test plan (on @riversand963's devserver)

$COMPILE_WITH_ASAN=1 make -j32 all && sleep 1 && make check

All tests pass.

riversand963 · 2019-06-21T16:16:39Z

Thanks @mrambacher for this PR. Can you rebase it as well? I will take a look at this one first.

mrambacher · 2019-06-21T19:45:45Z

Should be fixed now. Let me know if there are any issues.

…

On Fri, Jun 21, 2019 at 12:17 PM Yanqin Jin ***@***.***> wrote: Thanks @mrambacher <https://github.com/mrambacher> for this PR. Can you rebase it as well? I will take a look at this one first. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5293?email_source=notifications&email_token=AABCTBDM6P3PFGD2AFZOTMLP3T5JZA5CNFSM4HL4365KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYI5BNI#issuecomment-504484021>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABCTBDK36UTOIGURJJ6RA3P3T5JZANCNFSM4HL4365A> .

mrambacher · 2019-07-01T14:28:09Z

Any progress? Let me know if there is anything I can do to help. I will be at the Meetup next week if you want to meet in person before or after that. I will be presenting what we are trying to do with these changes during the meetup. Let m know if you are available and want to talk.

…

On Fri, Jun 21, 2019 at 3:45 PM Mark Rambacher ***@***.***> wrote: Should be fixed now. Let me know if there are any issues. On Fri, Jun 21, 2019 at 12:17 PM Yanqin Jin ***@***.***> wrote: > Thanks @mrambacher <https://github.com/mrambacher> for this PR. Can you > rebase it as well? I will take a look at this one first. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#5293?email_source=notifications&email_token=AABCTBDM6P3PFGD2AFZOTMLP3T5JZA5CNFSM4HL4365KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYI5BNI#issuecomment-504484021>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AABCTBDK36UTOIGURJJ6RA3P3T5JZANCNFSM4HL4365A> > . >

riversand963 · 2019-07-01T16:04:48Z

@mrambacher . sorry I was unable to review it last week because something else came up and got in my way. I will try to take a look this week.
Sure we can meet in person during the meetup. Look forward to the talk!

riversand963

Thanks @mrambacher for the PR. I took a pass and left some comments.

options/options_test.cc

tools/db_bench_tool.cc

include/rocksdb/comparator.h

include/rocksdb/utilities/object_registry.h

utilities/object_registry.cc

include/rocksdb/utilities/object_registry.h

utilities/object_registry.cc

riversand963 · 2019-07-13T00:25:45Z

Could you fix Travis and AppVeyor errors, please? @mrambacher

mrambacher · 2019-07-15T12:29:01Z

I pushed out a new changeset that should resolve all of the issues you have brought up. Let me know if there is anything further. If you need me to, I can squash the branch, but thought it might be easier for you if the changes were not for the next round of review.

riversand963 · 2019-07-17T18:19:03Z

Thanks for fixing the tests. I am going to take a look today.

mrambacher · 2019-07-18T15:01:22Z

I pushed out a second changeset in this branch (to add a Env::LoadEnv). If you are okay with these two changes, I will merge them into one prior to final approval.

facebook-github-bot

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

riversand963

I think the overall design LGTM. However I have a few questions. Furthermore, have you run make all && make check? It still fails on my devserver. I may have time to take a look this week, but it would be great if you can give it a try.

options/options_helper.cc

utilities/object_registry.cc

riversand963 · 2019-07-18T22:12:52Z

src.mk

@@ -184,6 +184,7 @@ LIB_SOURCES =                                                   \
  utilities/debug.cc                                            \
  utilities/env_mirror.cc                                       \
  utilities/env_timed.cc                                        \
+  utilities/object_registry.cc                                  \


Also, these lines should be sorted in alphabetic order.

mrambacher · 2019-07-19T15:12:44Z

I have been testing by doing a "make dbg" and running all of those tests, which passes.

When I try a "make check", I see an error but cannot tell what is failing. How do I determine what is failing from make check?

riversand963 · 2019-07-19T17:11:19Z

Will take a look at the failure. I think it is very close to the finishing line.
btw, looks like the description of this PR has duplicate text. Could you please revise a little please? Thanks.

riversand963 · 2019-07-19T17:49:38Z

here is a repro:

$DEBUG_LEVEL=2 make -j32 ldb
$mkdir -p /tmp/rocksdbtest/ldb_test
$./ldb --db=/tmp/rocksdbtest-148062/ldb_test/ put cf1_1 1 --create_if_missing
$./ldb --db=/tmp/rocksdbtest-148062/ldb_test/ put cf1_2 2 --create_if_missing
$gdb --args ./ldb --db=/tmp/rocksdbtest-148062/ldb_test/ put cf1_3 3 --try_load_options
(gdb)run
(it will abort printing something)
(gdb) bt                                                                                                                                                                                                                                             │ public:
#0  0x0000000000451e48 in rocksdb::UserComparatorWrapper::Name (this=0x7ffff66f0750) at ./util/user_comparator_wrapper.h:37                                                                                                                          │  explicit UserComparatorWrapper(const Comparator* const user_cmp)
#1  0x0000000000452054 in rocksdb::InternalKeyComparator::InternalKeyComparator (this=0x7ffff66f0740, c=0x0) at ./db/dbformat.h:192                                                                                                                  │      : user_comparator_(user_cmp) {}
#2  0x000000000044b435 in rocksdb::ColumnFamilyData::ColumnFamilyData (this=0x7ffff66f0700, id=0, name="default", _dummy_versions=0x7ffff66b1000, _table_cache=0x7ffff666c290, write_buffer_manager=0x7ffff664a190, cf_options=..., db_options=...,  │
    env_options=..., column_family_set=0x7ffff6609500, block_cache_tracer=0x7ffff66b28f8) at db/column_family.cc:439                                                                                                                                 │  ~UserComparatorWrapper() = default;
#3  0x000000000044f471 in rocksdb::ColumnFamilySet::CreateColumnFamily (this=0x7ffff6609500, name="default", id=0, dummy_versions=0x7ffff66b1000, options=...) at db/column_family.cc:1340                                                           │
#4  0x00000000006135bc in rocksdb::VersionSet::CreateColumnFamily (this=0x7ffff663a180, cf_options=..., edit=0x7fffffffb430) at db/version_set.cc:5184                                                                                               │  const Comparator* user_comparator() const { return user_comparator_; }
#5  0x000000000060defd in rocksdb::VersionSet::Recover (this=0x7ffff663a180, column_families=std::vector of length 1, capacity 1 = {...}, read_only=false) at db/version_set.cc:4253                                                                 │
#6  0x0000000000539af2 in rocksdb::DBImpl::Recover (this=0x7ffff66b2400, column_families=std::vector of length 1, capacity 1 = {...}, read_only=false, error_if_log_file_exist=false, error_if_data_exists_in_logs=false)                            │  int Compare(const Slice& a, const Slice& b) const override {
    at db/db_impl/db_impl_open.cc:378                                                                                                                                                                                                                │    PERF_COUNTER_ADD(user_key_comparison_count, 1);
#7  0x000000000053eea3 in rocksdb::DBImpl::Open (db_options=..., dbname="/tmp/rocksdbtest-148062/ldb_test/", column_families=std::vector of length 1, capacity 1 = {...}, handles=0x7fffffffc9d0, dbptr=0x7ffff66a8070, seq_per_batch=false,         │    return user_comparator_->Compare(a, b);
    batch_per_txn=true) at db/db_impl/db_impl_open.cc:1299                                                                                                                                                                                           │  }
#8  0x000000000053e40a in rocksdb::DB::Open (db_options=..., dbname="/tmp/rocksdbtest-148062/ldb_test/", column_families=std::vector of length 1, capacity 1 = {...}, handles=0x7fffffffc9d0, dbptr=0x7ffff66a8070)                                  │
    at db/db_impl/db_impl_open.cc:1196                                                                                                                                                                                                               │  bool Equal(const Slice& a, const Slice& b) const override {
#9  0x00000000008a5308 in rocksdb::LDBCommand::OpenDB (this=0x7ffff66a8000) at tools/ldb_cmd.cc:395                                                                                                                                                  │    PERF_COUNTER_ADD(user_key_comparison_count, 1);
#10 0x00000000008a45b3 in rocksdb::LDBCommand::Run (this=0x7ffff66a8000) at tools/ldb_cmd.cc:273                                                                                                                                                     │    return user_comparator_->Equal(a, b);
#11 0x00000000008c5324 in rocksdb::LDBCommandRunner::RunCommand (argc=6, argv=0x7fffffffde98, options=..., ldb_options=..., column_families=0x0) at tools/ldb_tool.cc:117                                                                            │  }
#12 0x00000000008c5458 in rocksdb::LDBTool::Run (this=0x7fffffffd8ff, argc=6, argv=0x7fffffffde98, options=..., ldb_options=..., column_families=0x0) at tools/ldb_tool.cc:129                                                                       │
#13 0x000000000040be5d in main (argc=6, argv=0x7fffffffde98) at tools/ldb.cc:12                                                                                                                                                                      │  const char* Name() const override { return user_comparator_->Name(); }
(gdb) p user_comparator_                                                                                                                                                                                                                             │
$1 = (const rocksdb::Comparator *) 0x0

facebook-github-bot · 2019-07-19T19:25:45Z

@mrambacher has updated the pull request. Re-import the pull request

mrambacher · 2019-07-19T19:27:23Z

This should now be fixed. I rebased and pushed the changes back out.

Can you tell me how/where you tracked down what was failing? I could not find that information in the logs to determine what was going wrong.

facebook-github-bot

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

facebook-github-bot · 2019-07-23T17:47:07Z

@mrambacher has updated the pull request. Re-import the pull request

facebook-github-bot

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

mrambacher · 2019-07-23T18:09:07Z

I have nothing further to add here. If you want me to rebase, I can but need to know where you want me to rebase from.

riversand963 · 2019-07-23T18:12:20Z

@mrambacher , cool! I think I can take from here.
I said "rebase" because I would like the PR to include my format fixes. Since now you do not have further stuff to add, I can land once the tests pass.

facebook-github-bot · 2019-07-23T19:37:02Z

@mrambacher has updated the pull request. Re-import the pull request

facebook-github-bot

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

facebook-github-bot · 2019-07-23T21:22:36Z

@mrambacher has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-07-23T21:23:39Z

@mrambacher has updated the pull request. Re-import the pull request

facebook-github-bot

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

…t error Fix bench tool to build in LITE mode on MAC Unused local variable warning

The ObjectRegistry class replaces the Registrar and NewCustomObjects. Objects are registered with the registry by Type (the class must implement the static const char *Type() method). This change is necessary for a few reasons: By having a class (rather than static template instances), the class can be passed between compilation units, meaning that objects could be registered and shared from a dynamic library with an executable. By having a class with instances, different units could have different objects registered. This could be useful if, for example, one Option allowed for a dynamic library and one did not. This change is necessary for a few reasons: By having a class (rather than static template instances), the class can be passed between compilation units, meaning that objects could be registered and shared from a dynamic library with an executable. By having a class with instances, different units could have different objects registered. This could be useful if, for example, one Option allowed for a dynamic library and one did not. When combined with some other PRs (being able to load shared libraries, a Configurable interface to configure objects to/from string), this code will allow objects in external shared libraries to be added to a RocksDB image at run-time, rather than requiring every new extension to be built into the main library and called explicitly by every program.

facebook-github-bot · 2019-07-23T21:27:38Z

@mrambacher has updated the pull request. Re-import the pull request

facebook-github-bot

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

facebook-github-bot · 2019-07-24T04:08:43Z

@riversand963 merged this pull request in cfcf045.

facebook#5293) Summary: The ObjectRegistry class replaces the Registrar and NewCustomObjects. Objects are registered with the registry by Type (the class must implement the static const char *Type() method). This change is necessary for a few reasons: - By having a class (rather than static template instances), the class can be passed between compilation units, meaning that objects could be registered and shared from a dynamic library with an executable. - By having a class with instances, different units could have different objects registered. This could be useful if, for example, one Option allowed for a dynamic library and one did not. When combined with some other PRs (being able to load shared libraries, a Configurable interface to configure objects to/from string), this code will allow objects in external shared libraries to be added to a RocksDB image at run-time, rather than requiring every new extension to be built into the main library and called explicitly by every program. Test plan (on riversand963's devserver) ``` $COMPILE_WITH_ASAN=1 make -j32 all && sleep 1 && make check ``` All tests pass. Pull Request resolved: facebook#5293 Differential Revision: D16363396 Pulled By: riversand963 fbshipit-source-id: fbe4acb615bfc11103eef40a0b288845791c0180

…… (#5293) Summary: The ObjectRegistry class replaces the Registrar and NewCustomObjects. Objects are registered with the registry by Type (the class must implement the static const char *Type() method). This change is necessary for a few reasons: - By having a class (rather than static template instances), the class can be passed between compilation units, meaning that objects could be registered and shared from a dynamic library with an executable. - By having a class with instances, different units could have different objects registered. This could be useful if, for example, one Option allowed for a dynamic library and one did not. When combined with some other PRs (being able to load shared libraries, a Configurable interface to configure objects to/from string), this code will allow objects in external shared libraries to be added to a RocksDB image at run-time, rather than requiring every new extension to be built into the main library and called explicitly by every program. Test plan (on riversand963's devserver) ``` $COMPILE_WITH_ASAN=1 make -j32 all && sleep 1 && make check ``` All tests pass. Pull Request resolved: facebook/rocksdb#5293 Differential Revision: D16363396 Pulled By: riversand963 fbshipit-source-id: fbe4acb615bfc11103eef40a0b288845791c0180 Signed-off-by: Changlong Chen <levisonchen@live.cn>

facebook-github-bot added the CLA Signed label May 9, 2019

mrambacher force-pushed the ObjectRegistry branch 3 times, most recently from eca7264 to 9be32d9 Compare June 14, 2019 13:55

mrambacher force-pushed the ObjectRegistry branch from 9be32d9 to 2d87f30 Compare June 14, 2019 22:10

mrambacher mentioned this pull request Jun 21, 2019

Create a Configurable base class for use in get/set values from properties and INI files #5361

Closed

mrambacher force-pushed the ObjectRegistry branch from 2d87f30 to a662502 Compare June 21, 2019 19:44

riversand963 self-requested a review June 21, 2019 20:09

mrambacher force-pushed the ObjectRegistry branch from a662502 to 73c06fe Compare June 24, 2019 16:35

mrambacher force-pushed the ObjectRegistry branch from 73c06fe to 3957068 Compare July 11, 2019 02:37

riversand963 reviewed Jul 11, 2019

View reviewed changes

riversand963 self-requested a review July 17, 2019 18:50

mrambacher force-pushed the ObjectRegistry branch from 2be7d7b to cc7e583 Compare July 18, 2019 14:59

facebook-github-bot reviewed Jul 18, 2019

View reviewed changes

riversand963 reviewed Jul 18, 2019

View reviewed changes

riversand963 self-requested a review July 19, 2019 17:09

mrambacher force-pushed the ObjectRegistry branch from cc7e583 to 569d0b4 Compare July 19, 2019 19:25

facebook-github-bot reviewed Jul 23, 2019

View reviewed changes

riversand963 force-pushed the ObjectRegistry branch from 644ad6f to 85650da Compare July 23, 2019 21:22

facebook-github-bot reviewed Jul 23, 2019

View reviewed changes

mrambacher and others added 8 commits July 23, 2019 14:26

Fix an issue with trace analyzer not building on the MAC b/c of forma…

439347d

…t error Fix bench tool to build in LITE mode on MAC Unused local variable warning

make format

5a2b3b1

Replace inttypes.h with cinttypes

4dd3a88

Minor fix

ec6c2aa

Update HISTORY.md

a829e39

Update buck

dc98a97

Fix a lint warning caused by tab

9355b22

riversand963 force-pushed the ObjectRegistry branch from a48855e to 9355b22 Compare July 23, 2019 21:27

facebook-github-bot reviewed Jul 23, 2019

View reviewed changes

facebook-github-bot closed this in cfcf045 Jul 24, 2019

facebook-github-bot added the Merged label Jul 24, 2019

mrambacher deleted the ObjectRegistry branch April 29, 2020 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The ObjectRegistry class replaces the Registrar and NewCustomObjects.… #5293

The ObjectRegistry class replaces the Registrar and NewCustomObjects.… #5293

mrambacher commented May 9, 2019 •

edited by riversand963

Loading

riversand963 commented Jun 21, 2019

mrambacher commented Jun 21, 2019 via email

mrambacher commented Jul 1, 2019 via email

riversand963 commented Jul 1, 2019

riversand963 left a comment

riversand963 commented Jul 13, 2019

mrambacher commented Jul 15, 2019

riversand963 commented Jul 17, 2019

mrambacher commented Jul 18, 2019

facebook-github-bot left a comment

riversand963 left a comment

riversand963 Jul 18, 2019

mrambacher commented Jul 19, 2019

riversand963 commented Jul 19, 2019

riversand963 commented Jul 19, 2019 •

edited

Loading

facebook-github-bot commented Jul 19, 2019

mrambacher commented Jul 19, 2019

facebook-github-bot left a comment

facebook-github-bot commented Jul 23, 2019

facebook-github-bot left a comment

mrambacher commented Jul 23, 2019

riversand963 commented Jul 23, 2019

facebook-github-bot commented Jul 23, 2019

facebook-github-bot left a comment

facebook-github-bot commented Jul 23, 2019

facebook-github-bot commented Jul 23, 2019

facebook-github-bot left a comment

facebook-github-bot commented Jul 23, 2019

facebook-github-bot left a comment

facebook-github-bot commented Jul 24, 2019

The ObjectRegistry class replaces the Registrar and NewCustomObjects.… #5293

The ObjectRegistry class replaces the Registrar and NewCustomObjects.… #5293

Conversation

mrambacher commented May 9, 2019 • edited by riversand963 Loading

riversand963 commented Jun 21, 2019

mrambacher commented Jun 21, 2019 via email

mrambacher commented Jul 1, 2019 via email

riversand963 commented Jul 1, 2019

riversand963 left a comment

Choose a reason for hiding this comment

riversand963 commented Jul 13, 2019

mrambacher commented Jul 15, 2019

riversand963 commented Jul 17, 2019

mrambacher commented Jul 18, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

riversand963 left a comment

Choose a reason for hiding this comment

riversand963 Jul 18, 2019

Choose a reason for hiding this comment

mrambacher commented Jul 19, 2019

riversand963 commented Jul 19, 2019

riversand963 commented Jul 19, 2019 • edited Loading

facebook-github-bot commented Jul 19, 2019

mrambacher commented Jul 19, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 23, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

mrambacher commented Jul 23, 2019

riversand963 commented Jul 23, 2019

facebook-github-bot commented Jul 23, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 23, 2019

facebook-github-bot commented Jul 23, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 23, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 24, 2019

mrambacher commented May 9, 2019 •

edited by riversand963

Loading

riversand963 commented Jul 19, 2019 •

edited

Loading