You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/09/18 08:43:33 UTC
[GitHub] [incubator-kvrocks] tufitko opened a new pull request, #885: feat: added disableWAL option
tufitko opened a new pull request, #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885
#878
also, I've updated Dockerfile with Prerequisite command, because it didn't build
C++ isn't my common lang, so any comments are welcome
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#discussion_r974119973
##########
src/config.cc:
##########
@@ -174,6 +174,7 @@ Config::Config() {
{"rocksdb.delayed_write_rate", false, new Int64Field(&RocksDB.delayed_write_rate, 0, 0, INT64_MAX)},
{"rocksdb.wal_ttl_seconds", true, new IntField(&RocksDB.WAL_ttl_seconds, 3*3600, 0, INT_MAX)},
{"rocksdb.wal_size_limit_mb", true, new IntField(&RocksDB.WAL_size_limit_MB, 16384, 0, INT_MAX)},
+ {"rocksdb.disableWAL", true, new YesNoField(&RocksDB.disableWAL, false)},
Review Comment:
I think `disable_wal` is more suitable in configuration file? since other options are all underscore-case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #885: feat: add rocksdb write_options to configuration
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#discussion_r975386657
##########
src/config.cc:
##########
@@ -603,6 +611,9 @@ Status Config::finish() {
auto s = rocksdb::Env::Default()->CreateDirIfMissing(name);
if (!s.ok()) return Status(Status::NotOK, s.ToString());
}
+ if (!slaveof_.empty() && RocksDB.write_options.disable_WAL) {
+ return Status(Status::NotOK, "cannot disable the WAL while using Master-Slave replication");
+ }
Review Comment:
I think we should check this in the `slaveof` command.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] mapleFU commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
mapleFU commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250234339
Hi, I think we can provide a `defaultRocksDBOption` or `writeWithDefault` in `storage`, I think just create default `WriteOptions` everywhere is not a good idea, and only those who know what rocksdb's options means can define them themselves
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#discussion_r973698900
##########
Dockerfile:
##########
@@ -15,20 +15,20 @@
# specific language governing permissions and limitations
# under the License.
-FROM ubuntu:focal as build
+FROM ubuntu:jammy as build
# workaround tzdata install hanging
ENV TZ=Asia/Shanghai
RUN ln -snf /usr/share/zoneinfo/$TZ /etc/localtime && echo $TZ > /etc/timezone
RUN apt update
-RUN apt install -y cmake make git autoconf libtool g++ python3
+RUN apt install -y git gcc g++ make cmake autoconf automake libtool python3 libssl-dev
Review Comment:
I pick this line to #887, since this PR may not be merged quickly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250242657
I also prefer to add the `WriteWithDefault` to avoid creating the option everywhere, then we can keep the `Write` method to be consistent with RocksDB.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tufitko commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tufitko commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250262936
So, I've reworked PR with `WriteWithDefault`. What about `Delete`? There was write_options and I removed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tufitko commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tufitko commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1251946076
@tanruixiang updated, I'm not sure I understood correctly.
I didn't add `timestamp` because I still think it's useless. also, there isn't it in newer versions of rocksdb.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250947048
> I think these parameters can be reserved, and synchronization in the config file(For example, adding an interface to change the `default writeoption`. If the configuration file is configured with `default writeoption`, then make corresponding modifications.). Of course, if it is not in the configuration file, the default value is used.
After this modification, if you want to `disableWAL`, you only need to modify the configuration file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#discussion_r973958907
##########
src/storage.cc:
##########
@@ -511,7 +512,7 @@ rocksdb::Status Storage::Write(const rocksdb::WriteOptions &options, rocksdb::Wr
updates->PutLogData(ServerLogData(kReplIdLog, replid_).Encode());
}
- return db_->Write(options, updates);
+ return db_->Write(write_opts_, updates);
Review Comment:
```suggestion
return db_->Write(options, updates);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#discussion_r974342286
##########
src/config.cc:
##########
@@ -174,6 +174,7 @@ Config::Config() {
{"rocksdb.delayed_write_rate", false, new Int64Field(&RocksDB.delayed_write_rate, 0, 0, INT64_MAX)},
{"rocksdb.wal_ttl_seconds", true, new IntField(&RocksDB.WAL_ttl_seconds, 3*3600, 0, INT_MAX)},
{"rocksdb.wal_size_limit_mb", true, new IntField(&RocksDB.WAL_size_limit_MB, 16384, 0, INT_MAX)},
+ {"rocksdb.disableWAL", true, new YesNoField(&RocksDB.disableWAL, false)},
Review Comment:
I also found this point, I think it should be inconsistent in RocksDB NOT the intent. `disable_wal` even `disable_WAL` is better than `disableWAL`, what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tufitko commented on a diff in pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tufitko commented on code in PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#discussion_r973714548
##########
Dockerfile:
##########
@@ -15,20 +15,20 @@
# specific language governing permissions and limitations
# under the License.
-FROM ubuntu:focal as build
+FROM ubuntu:jammy as build
# workaround tzdata install hanging
ENV TZ=Asia/Shanghai
RUN ln -snf /usr/share/zoneinfo/$TZ /etc/localtime && echo $TZ > /etc/timezone
RUN apt update
-RUN apt install -y cmake make git autoconf libtool g++ python3
+RUN apt install -y git gcc g++ make cmake autoconf automake libtool python3 libssl-dev
Review Comment:
ok! Then I remove it from my PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk merged pull request #885: feat: add rocksdb write_options to configuration
Posted by GitBox <gi...@apache.org>.
git-hulk merged PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1251977938
I prefer to expose only the options that may be used in practice. If we increase the number of options exposed, then we need to consider how kvrocks will behave if this option is used, and whether all aspects of the functionality in kvrocks will still work.
If we can't be sure of that, then just exposing `disbale_wal` is fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250903774
As @PragmaTwice said. I think it is possible to increase the configuration of default writeoption in the configuration. Doing this eliminates the need to recompile the code every time the writeoption is modified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#discussion_r973698900
##########
Dockerfile:
##########
@@ -15,20 +15,20 @@
# specific language governing permissions and limitations
# under the License.
-FROM ubuntu:focal as build
+FROM ubuntu:jammy as build
# workaround tzdata install hanging
ENV TZ=Asia/Shanghai
RUN ln -snf /usr/share/zoneinfo/$TZ /etc/localtime && echo $TZ > /etc/timezone
RUN apt update
-RUN apt install -y cmake make git autoconf libtool g++ python3
+RUN apt install -y git gcc g++ make cmake autoconf automake libtool python3 libssl-dev
Review Comment:
I pick this line to #887, since this PR may not be merged quickly. Thanks for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250263776
> So, I've reworked PR with `WriteWithDefault`. What about `Delete`? There was write_options and I removed it.
I think you can recover the WriteOptions parameter, and just expose a method `WriteOptions Storage::DefaultWriteOptions()`.
And you can also add something like `DeleteWithDefault(...) { Delete(DefaultWriteOptions(), ...) }` if you like.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tufitko commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tufitko commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250334440
> > So, I've reworked PR with `WriteWithDefault`. What about `Delete`? There was write_options and I removed it.
>
> I think you can recover the WriteOptions parameter, and just expose a method `WriteOptions Storage::DefaultWriteOptions()`.
>
> And you can also add something like `DeleteWithDefault(...) { Delete(DefaultWriteOptions(), ...) }` if you like.
reworked again 🙈
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tufitko commented on a diff in pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tufitko commented on code in PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#discussion_r974111876
##########
src/storage.cc:
##########
@@ -511,7 +512,7 @@ rocksdb::Status Storage::Write(const rocksdb::WriteOptions &options, rocksdb::Wr
updates->PutLogData(ServerLogData(kReplIdLog, replid_).Encode());
}
- return db_->Write(options, updates);
+ return db_->Write(write_opts_, updates);
Review Comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tufitko commented on a diff in pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tufitko commented on code in PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#discussion_r974110135
##########
src/storage.cc:
##########
@@ -511,7 +512,7 @@ rocksdb::Status Storage::Write(const rocksdb::WriteOptions &options, rocksdb::Wr
updates->PutLogData(ServerLogData(kReplIdLog, replid_).Encode());
}
- return db_->Write(options, updates);
+ return db_->Write(write_opts_, updates);
Review Comment:
ooops
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tufitko commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tufitko commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250937528
> As @PragmaTwice said. I think it is possible to increase the configuration of default writeoption in the configuration. Doing this eliminates the need to recompile the code every time the writeoption is modified.
hmmm, I can add `sync`, `no_slowdown`, `low_pri`, `memtable_insert_hint_per_batch`. But `ignore_missing_column_families` and `timestamp` looks useless to setting through the config. do you agree with this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tufitko commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tufitko commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1252006691
so, I need a decision about exposing write options.
I think `ignore_missing_column_families` and `timestamp` are unneeded for configuring. Others can be useful
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tufitko commented on a diff in pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tufitko commented on code in PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#discussion_r974166303
##########
src/config.cc:
##########
@@ -174,6 +174,7 @@ Config::Config() {
{"rocksdb.delayed_write_rate", false, new Int64Field(&RocksDB.delayed_write_rate, 0, 0, INT64_MAX)},
{"rocksdb.wal_ttl_seconds", true, new IntField(&RocksDB.WAL_ttl_seconds, 3*3600, 0, INT_MAX)},
{"rocksdb.wal_size_limit_mb", true, new IntField(&RocksDB.WAL_size_limit_MB, 16384, 0, INT_MAX)},
+ {"rocksdb.disableWAL", true, new YesNoField(&RocksDB.disableWAL, false)},
Review Comment:
`disable_wal` looks better, but in RocksDb it calls `disableWAL`. shouldn't we save the original name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #885: feat: add rocksdb write_options to configuration
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#discussion_r975414472
##########
src/config.cc:
##########
@@ -603,6 +611,9 @@ Status Config::finish() {
auto s = rocksdb::Env::Default()->CreateDirIfMissing(name);
if (!s.ok()) return Status(Status::NotOK, s.ToString());
}
+ if (!slaveof_.empty() && RocksDB.write_options.disable_WAL) {
+ return Status(Status::NotOK, "cannot disable the WAL while using Master-Slave replication");
+ }
Review Comment:
Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250945265
> > As @PragmaTwice said. I think it is possible to increase the configuration of default writeoption in the configuration. Doing this eliminates the need to recompile the code every time the writeoption is modified.
>
> hmmm, I can add `sync`, `no_slowdown`, `low_pri`, `memtable_insert_hint_per_batch`. But `ignore_missing_column_families` and `timestamp` looks useless to setting through the config. do you agree with this?
I think these parameters can be reserved, and synchronization in the config file(For example, adding an interface to change the `default writeoption`. If the configuration file is configured with `default writeoption`, then make corresponding modifications.). Of course, if it is not in the configuration file, the default value is used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tufitko commented on a diff in pull request #885: feat: add rocksdb write_options to configuration
Posted by GitBox <gi...@apache.org>.
tufitko commented on code in PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#discussion_r975403758
##########
src/config.cc:
##########
@@ -603,6 +611,9 @@ Status Config::finish() {
auto s = rocksdb::Env::Default()->CreateDirIfMissing(name);
if (!s.ok()) return Status(Status::NotOK, s.ToString());
}
+ if (!slaveof_.empty() && RocksDB.write_options.disable_WAL) {
+ return Status(Status::NotOK, "cannot disable the WAL while using Master-Slave replication");
+ }
Review Comment:
ok, moved to `Execution` of slaveof
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1252250689
There are some parameter settings in writeoption that should be safe, such as `sync`. I think it's okay to keep these parameters, and if the user uses a parameter in config file that we're not sure about, then we give a warning (I think for users unfamiliar with writeoption, they'll just use the default values, and users who modify those values are sure Is to have a certain understanding of rocksdb).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250225710
May I ask why change the storage interface. I think it is necessary to reserve this `WriteOptions` parameter. Not all types share the same storage's writeoption(Even though all are now using the same defaults, I think it's still necessary to keep).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tufitko commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tufitko commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250226604
@tanruixiang I can keep old Write interface, I've removed it because I didn't see non-default write options. Can you give an example of when this might be needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250227536
> @tanruixiang I can keep old Write interface, I've removed it because I didn't see non-default write options. Can you give an example of when this might be needed?
There are many options in `WriteOptions`, not just `disableWAL`. If someone needs to change other options like you, and use different options in different places, they will need this `WriteOptions` parameter. In particular, the `rocksdb::WriteOptions write_opts_ ` you add are private, which means we can't make changes in places where options might need to be changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tufitko commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tufitko commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250229837
🤔 @tanruixiang I think I need to return the old Write interface and add a method to return default write options from storage. Is it ok?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tanruixiang commented on pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#issuecomment-1250234821
> 🤔 @tanruixiang I think I need to return the old Write interface and add a method to return default write options from storage. Is it ok?
We can discuss it. @git-hulk @PragmaTwice What form do you think is the best?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #885: feat: added disableWAL option
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #885:
URL: https://github.com/apache/incubator-kvrocks/pull/885#discussion_r974119973
##########
src/config.cc:
##########
@@ -174,6 +174,7 @@ Config::Config() {
{"rocksdb.delayed_write_rate", false, new Int64Field(&RocksDB.delayed_write_rate, 0, 0, INT64_MAX)},
{"rocksdb.wal_ttl_seconds", true, new IntField(&RocksDB.WAL_ttl_seconds, 3*3600, 0, INT_MAX)},
{"rocksdb.wal_size_limit_mb", true, new IntField(&RocksDB.WAL_size_limit_MB, 16384, 0, INT_MAX)},
+ {"rocksdb.disableWAL", true, new YesNoField(&RocksDB.disableWAL, false)},
Review Comment:
I think `disable_wal` is more suitable in configuration file? since other options are all underscore-case and case-insensitive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org