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.

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

Add OptionsFormatter class and update to allow auto-logging of Options #651

Merged
merged 36 commits into from
Nov 15, 2023

Conversation

mrambacher
Copy link
Contributor

@mrambacher mrambacher commented Aug 25, 2023

This PR adds an OptionsFormatter to the ConfigOptions. This class allows the options to be written and read in different formats. Three formats initially supported for write are the existing Options-file properties format, the ";"-separated list of items, and writing the options to the log file.

By adding a LogOptionsFormatter, the system will be able to automatically log options to the file as they are added or removed. Currently, this involves a secondary step of updating the Dump method which is sometimes missed. Furthermore, some sub-classes may be skipped or not have their options printed at all.

Note that a later PR will use the LogOptionsFormatter to replace the code in Dump and GetPrintableOptions with the Options infrastructure and the LogOptionsFormatter

Converts an object to its serialized name-value properties.
This class allows the Options to be written to files in different formats.  Three formats are shown here - properties format, semi-colon separated, and log/dump-file formats.
@udi-speedb
Copy link
Contributor

@mrambacher - Please update when you have completed updating the branch. I will review only once the branch has all of the contents you intend to merge. Thanks

@udi-speedb
Copy link
Contributor

Why is the title "WIP: ..."?

@mrambacher mrambacher self-assigned this Aug 28, 2023
Similar function via ConfigOptions::ToMap. Body in DefaultOptionsFormatter::ToMap
Cleans up some stuff that was in the public API for use internally by moving it into the OptionsFormatter.

Eliminates use of ConfigOptions::delimiter, since it is now deprecated and replaced by the OptionsFormatter.
@udi-speedb udi-speedb requested review from ofriedma and removed request for udi-speedb September 24, 2023 02:18
@mrambacher mrambacher changed the title WIP: Add OptionsFormatter class and update to allow auto-logging of Options Add OptionsFormatter class and update to allow auto-logging of Options Oct 13, 2023
@udi-speedb
Copy link
Contributor

@ofriedma - Are you done reviewing the PR?

@ofriedma
Copy link
Contributor

ofriedma commented Nov 6, 2023

@ofriedma - Are you done reviewing the PR?

waiting for a fix to my comments

@mrambacher
Copy link
Contributor Author

@ofriedma - Are you done reviewing the PR?

waiting for a fix to my comments

Comments addressed. Please review again

@ofriedma
Copy link
Contributor

ofriedma commented Nov 8, 2023

@mrambacher There is a failure in the compile process

@ofriedma ofriedma self-requested a review November 8, 2023 09:43
Copy link
Contributor

@ofriedma ofriedma left a comment

Choose a reason for hiding this comment

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

see comments

@mrambacher
Copy link
Contributor Author

@mrambacher There is a failure in the compile process

License check issue. Believe it should be fixed or a bug in the license check for files that are not licensed.

@ofriedma
Copy link
Contributor

ofriedma commented Nov 9, 2023

Compiling locally, I'm getting:

In file included from options/options_formatter.cc:20:
./options/options_formatter_impl.h:56:15: error: ‘virtual std::string rocksdb::DefaultOptionsFormatter::ToString(const std::string&, char, const std::vector<std::__cxx11::basic_string >&) const’ was hidden [-Werror=overloaded-virtual]
56 | std::string ToString(const std::string& prefix, char separator,
| ^~~~~~~~
./options/options_formatter_impl.h:92:15: note: by ‘virtual std::string rocksdb::PropertiesOptionsFormatter::ToString(const std::string&, const rocksdb::OptionProperties&) const’
92 | std::string ToString(const std::string& prefix,
| ^~~~~~~~

@ofriedma ofriedma self-requested a review November 12, 2023 10:39
Copy link
Contributor

@ofriedma ofriedma left a comment

Choose a reason for hiding this comment

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

https://github.com/speedb-io/speedb/actions/runs/6839948302/job/18598695568

There is a compile issue again in the CI:

In file included from ../options/options_formatter.cc:20:
../options/options_formatter_impl.h:56:15: error: 'virtual std::string rocksdb::DefaultOptionsFormatter::ToString(const string&, char, const std::vector<std::__cxx11::basic_string >&) const' was hidden [-Werror=overloaded-virtual]
56 | std::string ToString(const std::string& prefix, char separator,
| ^~~~~~~~
../options/options_formatter_impl.h:92:15: note: by 'virtual std::string rocksdb::PropertiesOptionsFormatter::ToString(const string&, const rocksdb::OptionProperties&) const'
92 | std::string ToString(const std::string& prefix,
| ^~~~~~~~
cc1plus: all warnings being treated as errors
ninja: job failed: ccache /usr/bin/c++ -DBZIP2 -DGFLAGS=1 -DGFLAGS_IS_A_DLL=0 -DHAVE_PCLMUL -DHAVE_SSE42 -DLZ4 -DOS_LINUX -DROCKSDB_FALLOCATE_PRESENT -DROCKSDB_LIB_IO_POSIX -DROCKSDB_MALLOC_USABLE_SIZE -DROCKSDB_NO_DYNAMIC_EXTENSION -DROCKSDB_PLATFORM_POSIX -DROCKSDB_PORTABLE -DROCKSDB_RANGESYNC_PRESENT -DROCKSDB_SCHED_GETCPU_PRESENT -DSNAPPY -DZLIB -DZSTD -I../ -I../include -isystem ../third-party/gtest-1.8.1/fused-src -W -Wextra -Wall -pthread -Wsign-compare -Wshadow -Wno-unused-parameter -Wno-unused-variable -Woverloaded-virtual -Wnon-virtual-dtor -Wno-missing-field-initializers -Wno-strict-aliasing -Wno-invalid-offsetof -fno-omit-frame-pointer -momit-leaf-frame-pointer -Werror -fno-builtin-memcmp -O3 -DNDEBUG -fno-rtti -fPIC -std=gnu++17 -MD -MT CMakeFiles/speedb.dir/options/options_parser.cc.o -MF CMakeFiles/speedb.dir/options/options_parser.cc.o.d -o CMakeFiles/speedb.dir/options/options_parser.cc.o -c ../options/options_parser.cc
In file included from ../options/options_parser.cc:33:
../options/options_formatter_impl.h:56:15: error: 'virtual std::string rocksdb::DefaultOptionsFormatter::ToString(const string&, char, const std::vector<std::__cxx11::basic_string >&) const' was hidden [-Werror=overloaded-virtual]
56 | std::string ToString(const std::string& prefix, char separator,
| ^~~~~~~~
../options/options_formatter_impl.h:92:15: note: by 'virtual std::string rocksdb::PropertiesOptionsFormatter::ToString(const string&, const rocksdb::OptionProperties&) const'
92 | std::string ToString(const std::string& prefix,
| ^~~~~~~~

ofriedma
ofriedma previously approved these changes Nov 15, 2023
@ofriedma
Copy link
Contributor

@mrambacher there is a compile error here:
../db/db_options_test.cc:64:12: error: local variable 'mutable_map' will be copied despite being returned by name [-Werror,-Wreturn-std-move]
return mutable_map;
^~~~~~~~~~~
../db/db_options_test.cc:64:12: note: call 'std::move' explicitly to avoid copying
return mutable_map;
^~~~~~~~~~~
std::move(mutable_map)
../db/db_options_test.cc:76:12: error: local variable 'mutable_map' will be copied despite being returned by name [-Werror,-Wreturn-std-move]
return mutable_map;
^~~~~~~~~~~
../db/db_options_test.cc:76:12: note: call 'std::move' explicitly to avoid copying
return mutable_map;
^~~~~~~~~~~
std::move(mutable_map)

@udi-speedb udi-speedb merged commit 1c6a506 into speedb-io:main Nov 15, 2023
10 of 11 checks passed
udi-speedb pushed a commit that referenced this pull request Nov 19, 2023
#651)

This PR adds an OptionsFormatter to the ConfigOptions. This class allows the options to be written and read in different formats. Three formats initially supported for write are the existing Options-file properties format, the ";"-separated list of items, and writing the options to the log file.

By adding a LogOptionsFormatter, the system will be able to automatically log options to the file as they are added or removed. Currently, this involves a secondary step of updating the Dump method which is sometimes missed. Furthermore, some sub-classes may be skipped or not have their options printed at all.
udi-speedb pushed a commit that referenced this pull request Nov 23, 2023
#651)

This PR adds an OptionsFormatter to the ConfigOptions. This class allows the options to be written and read in different formats. Three formats initially supported for write are the existing Options-file properties format, the ";"-separated list of items, and writing the options to the log file.

By adding a LogOptionsFormatter, the system will be able to automatically log options to the file as they are added or removed. Currently, this involves a secondary step of updating the Dump method which is sometimes missed. Furthermore, some sub-classes may be skipped or not have their options printed at all.
udi-speedb pushed a commit that referenced this pull request Dec 6, 2023
#651)

This PR adds an OptionsFormatter to the ConfigOptions. This class allows the options to be written and read in different formats. Three formats initially supported for write are the existing Options-file properties format, the ";"-separated list of items, and writing the options to the log file.

By adding a LogOptionsFormatter, the system will be able to automatically log options to the file as they are added or removed. Currently, this involves a secondary step of updating the Dump method which is sometimes missed. Furthermore, some sub-classes may be skipped or not have their options printed at all.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants