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/17 11:28:19 UTC
[GitHub] [incubator-kvrocks] tanruixiang opened a new pull request, #882: Support DISK Command
tanruixiang opened a new pull request, #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882
It closes #874
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973576361
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
Review Comment:
There are too many duplicate code snippets in this file, I think you can think about how to reduce/eliminate them.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1250326915
> It's not only small cases where problems can arise. Sometimes a larger value also has a situation where the result is 0. For example, the failure of this CI has a relatively large value but get key size is zero. In this case ,we Lpush 1000 number value but get 0 size.[big value but get size 0 case](https://github.com/apache/incubator-kvrocks/actions/runs/3077380954/jobs/4972217783)
OK, got it. I need to read this PR first and try to reproduce it on my side.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1250325176
> > > > @PragmaTwice @git-hulk @torwig I noticed a problem. In the following tests. When we write a value with only one bit(like 'a'). Occasionally, the size of the key we get is 0. It occurs frequently when the value is very small. I guess the reason for this problem is that sync in writeoption defaults to false. Or because the `GetApproximateSizes` can only yield approximate values, which may be ignored for small values.
> > > > ```
> > > > std::vector<int> value_size{1, 1024};
> > > > for(auto &p : value_size){
> > > > EXPECT_TRUE(string->Set(key_, std::string(p, 'a')).ok());
> > > > std::string got_value;
> > > > EXPECT_TRUE(string->Get(key_, &got_value).ok());
> > > > EXPECT_EQ(got_value, std::string(p, 'a'));
> > > > std::string value;
> > > > string->Get(key_, &value);
> > > > uint64_t result = 0;
> > > > EXPECT_TRUE(disk->GetKeySize(key_, kRedisString, &result).ok());
> > > > EXPECT_GE(result, p);
> > > > }
> > > > ```
> > >
> > >
> > > @git-hulk Hi. Do you have time to look at this question? Because the approximate usage space is obtained, it may encounter a situation of 0 during testing. Increasing the sleep time also doesn't completely solve the problem. It seems that the sync method is not supported either.
> >
> >
> > Did you enable the `include_memtabtles` in SizeApproximationOptions?
>
> Of course yes.
>
> ```
> this->option_.include_memtabtles = true;
> this->option_.include_files = true;
> ```
OK, I need to take some time to inspect the reason. But I think it's ok to skip this corner case first since we should NOT care if the usage is small enough.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1250095278
@PragmaTwice Hi. Do you have any more suggestions on reducing redundant code?
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985057918
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <memory>
+#include <string>
+#include <vector>
+#include <gtest/gtest.h>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+#include "redis_stream.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ delete storage_;
Review Comment:
And of course without deleting existing config instance.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1264824161
> @tanruixiang Hi, I've done some tests locally. For example, if I set `x` to a string of length 50 and then use `disk` command - I get something about 80 as a result. Then I restart `kvrocks` and repeat `disk` command. And this time the result is something about 1500. Is it some kind of approximation by the `rocksdb`?
Hi. I think it's as expected. It should be the reason for the statistical granularity of rocksdb. You can see in [there](https://github.com/facebook/rocksdb/issues/10733).
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1274179015
> We also need to update our website :) @git-hulk
Do you want to do it? If you don't have time, I can update the website later today.
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985057773
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <memory>
+#include <string>
+#include <vector>
+#include <gtest/gtest.h>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+#include "redis_stream.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ delete storage_;
Review Comment:
> Have you tried to use this code without deleting and creating new storage? `config_` is passed through the pointer and it is used mainly within the `Open()` call.
It seems that there is no interface to directly modify the config. Will it cause a memory leak if I don't delete 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] tanruixiang commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985059402
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <memory>
+#include <string>
+#include <vector>
+#include <gtest/gtest.h>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+#include "redis_stream.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ delete storage_;
Review Comment:
> Now I see. The TestBase constructor calls storage_->Open()
Yes. My guess is that there are some resources that must be destructed to be freed, so it can't just be reopened.
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985057892
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <memory>
+#include <string>
+#include <vector>
+#include <gtest/gtest.h>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+#include "redis_stream.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ delete storage_;
Review Comment:
I was thinking about removing the line `storage_ = new Engine::Storage(config_);` Does it work?
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r977599875
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ return GetStringSize(ns_key, key_size);
+ case RedisType::kRedisHash:
+ return GetHashSize(ns_key, key_size);
+ case RedisType::kRedisBitmap:
+ return GetBitmapSize(ns_key, key_size);
+ case RedisType::kRedisList:
+ return GetListSize(ns_key, key_size);
+ case RedisType::kRedisSet:
+ return GetSetSize(ns_key, key_size);
+ case RedisType::kRedisSortedint:
+ return GetSortedintSize(ns_key, key_size);
+ case RedisType::kRedisZSet:
+ return GetZsetSize(ns_key, key_size);
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
Review Comment:
> @tanruixiang I've added Stream design here [apache/incubator-kvrocks-website#17](https://github.com/apache/incubator-kvrocks-website/pull/17) .
Thank you very much! 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] git-hulk commented on pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1254966409
> > OK, got it. I need to read this PR first and try to reproduce it on my side.
>
> @git-hulk I used the parameters configured for `GetApproximateSizes` testing in `rocksdb`. However, there is still a probability of zero results. Do you have any suggestions?
Sorry for missing this comment, I have no enough to inspect this case yet.
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973583119
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft ,
Review Comment:
I'd recommend making the `key_size` argument the last one because it is the output one and it is passed by a pointer.
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973595125
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ rocksdb::Status s;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ s = GetStringSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisHash:
+ s = GetHashSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisBitmap:
+ s = GetBitmapSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisList:
+ s = GetListSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSet:
+ s = GetSetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSortedint:
+ s = GetSortedintSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisZSet:
+ s = GetZsetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisNone:
+ s = GetHashSize(ns_key, key_size);
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ break;
Review Comment:
thank you. It has been 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] PragmaTwice commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973602136
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ rocksdb::Status s;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ s = GetStringSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisHash:
+ s = GetHashSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisBitmap:
+ s = GetBitmapSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisList:
+ s = GetListSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSet:
+ s = GetSetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSortedint:
+ s = GetSortedintSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisZSet:
+ s = GetZsetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
+ }
+ return s;
+}
+rocksdb::Status Disk::GetStringSize(const Slice &ns_key, uint64_t *key_size) {
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key.ToString() + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &ns_key, uint64_t *key_size) {
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+
+rocksdb::Status Disk::GetSetSize(const Slice &ns_key, uint64_t *key_size) {
+ SetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetListSize(const Slice &ns_key, uint64_t *key_size) {
+ ListMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisList, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string buf;
+ PutFixed64(&buf, metadata.head);
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size, buf);
+}
+
+rocksdb::Status Disk::GetZsetSize(const Slice &ns_key, uint64_t *key_size) {
+ ZSetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisZSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string score_bytes;
+ PutDouble(&score_bytes, kMinScore);
+ s = GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kZSetScoreColumnFamilyName),
+ key_size, score_bytes, score_bytes);
+ if (!s.ok()) return s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetBitmapSize(const Slice &ns_key, uint64_t *key_size) {
+ BitmapMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisBitmap, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Review Comment:
Oh you are right, a factory function is needed for `Metadata`. So weird since the ctor of `Metadata` is public, I think protected is enough. It is a mess while `Metadata` itself is contructed by users.
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985905516
##########
tests/gocase/unit/disk/disk_test.go:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package disk
+
+import (
+ "context"
+ "strconv"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func TestDisk(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{
+ "rocksdb.compression": "no",
+ "rocksdb.write_buffer_size": "1",
+ })
+ defer srv.Close()
+ ctx := context.Background()
+ rdb := srv.NewClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+ estimationFactor := 0.5
+ t.Run("Disk usage String", func(t *testing.T) {
+ require.NoError(t, rdb.Set(ctx, "stringkey", "aaaaaaaaaaaaaaaaaaaaaaaa", 0).Err())
+ val, err := rdb.Do(ctx, "Disk", "usage", "stringkey").Int()
+ require.NoError(t, err)
+ require.GreaterOrEqual(t, val, 1)
Review Comment:
@PragmaTwice Now my judgment is that the result will not exceed the upper bound or the lower bound within an order of magnitude
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973584006
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft ,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(this->option, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetStringSize(const Slice &user_key, uint64_t *key_size) {
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key + static_cast<char>(0)));
+ return db_->GetApproximateSizes(this->option, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+
+rocksdb::Status Disk::GetSetSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetListSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ ListMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisList, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string buf;
+ PutFixed64(&buf, metadata.head);
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size, buf);
+}
+
+rocksdb::Status Disk::GetZsetSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
Review Comment:
To be consistent with the rest of Kvrock's code, please use the indentation of 2 spaces instead of 4.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973585761
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetStringSize(const Slice &user_key, uint64_t *key_size) {
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Review Comment:
These code appears in almost every method in this file, which is like a copy-and-paste programming pattern, so I think you can just define something like
```
rocksdb::Status GetHashSize(auto ns_key, auto metadata, auto key_size) // fill these types
```
and then
```
RedisType type = ...;
std::string ns_key;
AppendNamespacePrefix(user_key, &ns_key);
Metadata metadata(type, false);
rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
switch (redisType) {
case kRedisHash: GetHashSize(...); break;
case kRedisSomething: GetSomethingSize(...)
...
}
```
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973586125
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetStringSize(const Slice &user_key, uint64_t *key_size) {
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Review Comment:
Or you can even do not define one method per redis type, I think it is ok to just implement it in the switch clause.
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973593959
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <gtest/gtest.h>
+#include <memory>
+#include <vector>
+#include <thread>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ }
+ ~RedisDiskTest() = default;
+
+protected:
+};
+
+TEST_F(RedisDiskTest, StringDisk) {
+ key_ = "stringdisk_key";
+ std::unique_ptr<Redis::String> string = Util::MakeUnique<Redis::String>(storage_, "disk_ns_string");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_string");
+ std::vector<int> value_size{1, 1024};
+ for(auto &p : value_size){
+ EXPECT_TRUE(string->Set(key_, std::string(p, 'a')).ok());
+ std::string got_value;
+ EXPECT_TRUE(string->Get(key_, &got_value).ok());
+ EXPECT_EQ(got_value, std::string(p, 'a'));
+ std::string value;
+ string->Get(key_, &value);
+ uint64_t result = 0;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisString, &result).ok());
+ EXPECT_GE(result, p);
+ break;
+ }
+ string->Del(key_);
+}
+
+TEST_F(RedisDiskTest, HashDisk) {
+ std::unique_ptr<Redis::Hash> hash = Util::MakeUnique<Redis::Hash>(storage_, "disk_ns_hash");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_hash");
+ key_ = "hashdisk_key";
+ fields_ = {"hashdisk_kkey1", "hashdisk_kkey2"};
+ values_.resize(2);
+ std::vector<int>value_size{1, 1024};
+ for(int i = 0 ;i < int(fields_.size()); i++){
+ values_[i] = std::string(value_size[i],'a');
+ }
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(fields_.size()); i++) {
+ sum += uint64_t(fields_[i].size()) + values_[i].size();
+ rocksdb::Status s = hash->Set(key_, fields_[i], values_[i], &ret);
+ EXPECT_TRUE(s.ok() && ret == 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisHash, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ hash->Del(key_);
+}
+
+TEST_F(RedisDiskTest, SetDisk) {
+ std::unique_ptr<Redis::Set> set = Util::MakeUnique<Redis::Set>(storage_, "disk_ns_set");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_set");
+ key_ = "setdisk_key";
+ values_.resize(2);
+ std::vector<int>value_size{1, 1024};
+ for(int i = 0;i < int(values_.size()); i++){
+ values_[i] = std::string(value_size[i],'a');
+ }
+
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(values_.size()); i++) {
+ std::vector<Slice>tmpv{values_[i]};
+ sum += values_[i].size();
+ rocksdb::Status s = set->Add(key_, tmpv, &ret);
+ EXPECT_TRUE(s.ok() && ret == 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisSet, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ set->Del(key_);
+}
+
+
+TEST_F(RedisDiskTest, ListDisk) {
+ std::unique_ptr<Redis::List> list = Util::MakeUnique<Redis::List>(storage_, "disk_ns_list");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_list");
+ key_ = "listdisk_key";
+ values_.resize(2);
+ std::vector<int>value_size{1,1024};
+ for(int i = 0;i < int(values_.size()); i++){
+ values_[i] = std::string(value_size[i],'a');
+ }
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(values_.size()); i++) {
+ std::vector<Slice>tmpv{values_[i]};
+ sum += values_[i].size();
+ rocksdb::Status s = list->Push(key_, tmpv, false, &ret);
+ EXPECT_TRUE(s.ok() && ret == i + 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisList, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ list->Del(key_);
+}
+
+TEST_F(RedisDiskTest, ZsetDisk) {
+ std::unique_ptr<Redis::ZSet> zset = Util::MakeUnique<Redis::ZSet>(storage_, "disk_ns_zet");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_zet");
+ key_ = "zsetdisk_key";
+ std::vector<MemberScore> mscores(2);
+ std::vector<int>value_size{1,1024};
+ for(int i = 0;i < int(value_size.size()); i++){
+ mscores[i].member = std::string(value_size[i],'a');
+ mscores[i].score = 1.0 * value_size[int(values_.size()) - i - 1];
+ }
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(mscores.size()); i++) {
+ std::vector<MemberScore> tmpv{mscores[i]};
+ sum += 2 * mscores[i].member.size();
+ rocksdb::Status s = zset->Add(key_, 0, &tmpv, &ret);
+ EXPECT_TRUE(s.ok() && ret == 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisZSet, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ zset->Del(key_);
+}
+
+TEST_F(RedisDiskTest, BitmapDisk) {
+ std::unique_ptr<Redis::Bitmap> bitmap = Util::MakeUnique<Redis::Bitmap>(storage_, "disk_ns_bitmap");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_bitmap");
+ key_ = "bitmapdisk_key";
+ uint32_t offsets[] = {0, 123, 1024*8, 1024*8+1, 3*1024*8, 3*1024*8+1};
+
+ for (int i = 0; i < 6 ;i++) {
+ bool bit = false;
+ bitmap->GetBit(key_, offsets[i], &bit);
+ EXPECT_FALSE(bit);
+ bitmap->SetBit(key_, offsets[i], true, &bit);
+ bitmap->GetBit(key_, offsets[i], &bit);
+ EXPECT_TRUE(bit);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisBitmap, &key_size).ok());
+ EXPECT_GE(key_size, i + 1);
+ }
+ bitmap->Del(key_);
+}
+
+TEST_F(RedisDiskTest, SortedintDisk) {
+ std::unique_ptr<Redis::Sortedint> sortedint = Util::MakeUnique<Redis::Sortedint>(storage_, "disk_ns_sortedint");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_sortedint");
+ key_ = "sortedintdisk_key";
+ int ret;
+ for(int i=0;i<10;i++){
Review Comment:
I noticed that the running results of ci are somewhat different from those of my native machine. At the same time, because this is a draft pr, I am using ci for testing.I found that a byte of data is often not detected due to the problem with the settings of Writebatch(sync is default false). So don't worry. I'll fix it after the test. Thanks again for your serious review
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <gtest/gtest.h>
+#include <memory>
+#include <vector>
+#include <thread>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ }
+ ~RedisDiskTest() = default;
+
+protected:
+};
+
+TEST_F(RedisDiskTest, StringDisk) {
+ key_ = "stringdisk_key";
+ std::unique_ptr<Redis::String> string = Util::MakeUnique<Redis::String>(storage_, "disk_ns_string");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_string");
+ std::vector<int> value_size{1, 1024};
+ for(auto &p : value_size){
+ EXPECT_TRUE(string->Set(key_, std::string(p, 'a')).ok());
+ std::string got_value;
+ EXPECT_TRUE(string->Get(key_, &got_value).ok());
+ EXPECT_EQ(got_value, std::string(p, 'a'));
+ std::string value;
+ string->Get(key_, &value);
+ uint64_t result = 0;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisString, &result).ok());
+ EXPECT_GE(result, p);
+ break;
Review Comment:
> I'm wondering why this operator (`break`) is here. With it, the loop will run only one time. But if you want to do something just one time, there is no reason to introduce the loop.
I noticed that the running results of ci are somewhat different from those of my native machine. At the same time, because this is a draft pr, I am using ci for testing.I found that a byte of data is often not detected due to the problem with the settings of Writebatch(sync is default false). So don't worry. I'll fix it after the test. Thanks again for your serious review
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1250323852
> > @PragmaTwice @git-hulk @torwig I noticed a problem. In the following tests. When we write a value with only one bit(like 'a'). Occasionally, the size of the key we get is 0. It occurs frequently when the value is very small. I guess the reason for this problem is that sync in writeoption defaults to false. Or because the `GetApproximateSizes` can only yield approximate values, which may be ignored for small values.
> > ```
> > std::vector<int> value_size{1, 1024};
> > for(auto &p : value_size){
> > EXPECT_TRUE(string->Set(key_, std::string(p, 'a')).ok());
> > std::string got_value;
> > EXPECT_TRUE(string->Get(key_, &got_value).ok());
> > EXPECT_EQ(got_value, std::string(p, 'a'));
> > std::string value;
> > string->Get(key_, &value);
> > uint64_t result = 0;
> > EXPECT_TRUE(disk->GetKeySize(key_, kRedisString, &result).ok());
> > EXPECT_GE(result, p);
> > }
> > ```
>
> @git-hulk Hi. Do you have time to look at this question? Because the approximate usage space is obtained, it may encounter a situation of 0 during testing. Increasing the sleep time also doesn't completely solve the problem. It seems that the sync method is not supported either.
Did you enable the `include_memtabtles` in SizeApproximationOptions?
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r974156124
##########
src/redis_disk.h:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <string>
+#include "redis_db.h"
+#include "redis_metadata.h"
+
+namespace Redis {
+
+class Disk : public Database {
+ public:
+ explicit Disk(Engine::Storage *storage, const std::string &ns): Database(storage, ns) {
+ this->option_.include_memtabtles = true;
+ this->option_.include_files = true;
+ }
+ rocksdb::Status GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft = Slice(),
+ Slice subkeyright = Slice());
+ rocksdb::Status GetStringSize(const Slice &user_key, uint64_t *key_size);
Review Comment:
> I think `GetApproximateKeySize` should be a virtual function and every data type should rewrite it
Using virtual functions is also a good approach. May I ask using virtual functions still require switch to create different objects? If so,that looks like it's about the same as my current implementation. Maybe my c++ is not proficient enough, do you have a better solution? Thank you for your review.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1253863957
> OK, got it. I need to read this PR first and try to reproduce it on my side.
@git-hulk I used the parameters configured for `GetApproximateSizes` testing in `rocksdb`. However, there is still a probability of zero results. Do you have any suggestions?
--
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] caipengbo commented on pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
caipengbo commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1274128551
LGTM
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973670527
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ rocksdb::Status s;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ s = GetStringSize(ns_key, key_size);
Review Comment:
You can return the status directly. BTW the switch block is not aligned well.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1250314669
> @PragmaTwice @git-hulk @torwig I noticed a problem. In the following tests. When we write a value with only one bit(like 'a'). Occasionally, the size of the key we get is 0. It occurs frequently when the value is very small. I guess the reason for this problem is that sync in writeoption defaults to false. Or because the `GetApproximateSizes` can only yield approximate values, which may be ignored for small values.
>
> ```
> std::vector<int> value_size{1, 1024};
> for(auto &p : value_size){
> EXPECT_TRUE(string->Set(key_, std::string(p, 'a')).ok());
> std::string got_value;
> EXPECT_TRUE(string->Get(key_, &got_value).ok());
> EXPECT_EQ(got_value, std::string(p, 'a'));
> std::string value;
> string->Get(key_, &value);
> uint64_t result = 0;
> EXPECT_TRUE(disk->GetKeySize(key_, kRedisString, &result).ok());
> EXPECT_GE(result, p);
> }
> ```
@git-hulk Hi. Do you have time to look at this question? Because the approximate usage space is obtained, it may encounter a situation of 0 during testing. Increasing the sleep time also doesn't completely solve the problem. It seems that the sync method is not supported either.
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973581864
##########
src/redis_disk.h:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <string>
+#include "redis_db.h"
+#include "redis_metadata.h"
+
+namespace Redis {
+
+class Disk : public Database {
+ public:
+ explicit Disk(Engine::Storage *storage, const std::string &ns): Database(storage, ns) {
+ this->option.include_memtabtles = true;
+ this->option.include_files = true;
+ }
+ rocksdb::Status GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft = Slice(),
+ Slice subkeyright = Slice());
+ rocksdb::Status GetStringSize(const Slice &user_key, uint64_t *key_size);
+ rocksdb::Status GetHashSize(const Slice &user_key, uint64_t *key_size);
+ rocksdb::Status GetSetSize(const Slice &user_key, uint64_t *key_size);
+ rocksdb::Status GetListSize(const Slice &user_key, uint64_t *key_size);
+ rocksdb::Status GetZsetSize(const Slice &user_key, uint64_t *key_size);
+ rocksdb::Status GetBitmapSize(const Slice &user_key, uint64_t *key_size);
+ rocksdb::Status GetSortedintSize(const Slice &user_key, uint64_t *key_size);
+ private:
+ rocksdb::SizeApproximationOptions option;
Review Comment:
Across Kvrocks codebase private members of the class are named with the `_` suffix. So you can rename `option` to `option` and omit using `this->option` everywhere.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973576361
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
Review Comment:
Hi @tanruixiang , thanks for you contribution!
I noticed that there are too many duplicate code snippets in this file, I think you can think about how to reduce/eliminate them.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973585761
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetStringSize(const Slice &user_key, uint64_t *key_size) {
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Review Comment:
These code appears in almost every method in this file, so I think you can just define something like
```
rocksdb::Status GetHashSize(auto ns_key, auto metadata, auto key_size) // fill these types
```
and then
```
RedisType type = ...;
std::string ns_key;
AppendNamespacePrefix(user_key, &ns_key);
Metadata metadata(type, false);
rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
switch (redisType) {
case kRedisHash: GetHashSize(...); break;
case kRedisSomething: GetSomethingSize(...)
}
```
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973593941
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <gtest/gtest.h>
Review Comment:
It's not a big deal, but I'd recommend grouping headers of the standard library together (`chrono` after third-party `gtest` header)
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985059329
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <memory>
+#include <string>
+#include <vector>
+#include <gtest/gtest.h>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+#include "redis_stream.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ delete storage_;
Review Comment:
Now I see. The `TestBase` constructor calls `storage_->Open()`
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973601406
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ rocksdb::Status s;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ s = GetStringSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisHash:
+ s = GetHashSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisBitmap:
+ s = GetBitmapSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisList:
+ s = GetListSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSet:
+ s = GetSetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSortedint:
+ s = GetSortedintSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisZSet:
+ s = GetZsetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
+ }
+ return s;
+}
+rocksdb::Status Disk::GetStringSize(const Slice &ns_key, uint64_t *key_size) {
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key.ToString() + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &ns_key, uint64_t *key_size) {
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+
+rocksdb::Status Disk::GetSetSize(const Slice &ns_key, uint64_t *key_size) {
+ SetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetListSize(const Slice &ns_key, uint64_t *key_size) {
+ ListMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisList, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string buf;
+ PutFixed64(&buf, metadata.head);
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size, buf);
+}
+
+rocksdb::Status Disk::GetZsetSize(const Slice &ns_key, uint64_t *key_size) {
+ ZSetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisZSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string score_bytes;
+ PutDouble(&score_bytes, kMinScore);
+ s = GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kZSetScoreColumnFamilyName),
+ key_size, score_bytes, score_bytes);
+ if (!s.ok()) return s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetBitmapSize(const Slice &ns_key, uint64_t *key_size) {
+ BitmapMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisBitmap, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Review Comment:
We need to set different types of metadata variables. The code seems to be more unreadable if the virtual function mechanism 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] tanruixiang commented on pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1250096015
@PragmaTwice @git-hulk @torwig I noticed a problem. In the following tests. When we write a value with only one bit(like 'a'). Occasionally, the size of the key we get is 0. It occurs frequently when the value is very small. I guess the reason for this problem is that sync in writeoption defaults to false.
```
std::vector<int> value_size{1, 1024};
for(auto &p : value_size){
EXPECT_TRUE(string->Set(key_, std::string(p, 'a')).ok());
std::string got_value;
EXPECT_TRUE(string->Get(key_, &got_value).ok());
EXPECT_EQ(got_value, std::string(p, 'a'));
std::string value;
string->Get(key_, &value);
uint64_t result = 0;
EXPECT_TRUE(disk->GetKeySize(key_, kRedisString, &result).ok());
EXPECT_GE(result, p);
}
```
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1274127253
@PragmaTwice @caipengbo @torwig Do you have any suggestion on this PR? Or I will merge it if we have no further comment.
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973676712
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ rocksdb::Status s;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ s = GetStringSize(ns_key, key_size);
Review Comment:
Thank you, it has been 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] tanruixiang commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973576804
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
Review Comment:
Ok. Thank you for your review.
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973584777
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft ,
Review Comment:
Aha. I see that you can't just move that parameter to the end because there are two args with default values in the end.
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r974156124
##########
src/redis_disk.h:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <string>
+#include "redis_db.h"
+#include "redis_metadata.h"
+
+namespace Redis {
+
+class Disk : public Database {
+ public:
+ explicit Disk(Engine::Storage *storage, const std::string &ns): Database(storage, ns) {
+ this->option_.include_memtabtles = true;
+ this->option_.include_files = true;
+ }
+ rocksdb::Status GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft = Slice(),
+ Slice subkeyright = Slice());
+ rocksdb::Status GetStringSize(const Slice &user_key, uint64_t *key_size);
Review Comment:
> I think `GetApproximateKeySize` should be a virtual function and every data type should rewrite it
Using virtual functions is also a good approach. May I ask using virtual functions still require switch to create different objects? If so,that looks like it's about the same as my current implementation. Maybe my c++ is not proficient enough, do you have a better solution?
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985059220
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <memory>
+#include <string>
+#include <vector>
+#include <gtest/gtest.h>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+#include "redis_stream.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ delete storage_;
Review Comment:
Yes, you are right, that was my idea. If it crashes, discard my idea :)
--
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] caipengbo commented on pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
caipengbo commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1274177494
> Yes, @caipengbo you can help to update as well when you're free. https://github.com/apache/incubator-kvrocks-website/blob/main/docs/01-supported-commands.md
If I update the library, will the site automatically update?
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1274180367
> > We also need to update our website :) @git-hulk
>
> Do you want to do it? If you don't have time, I can update the website later today.
Cool, thanks. @tanruixiang
--
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] caipengbo commented on pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
caipengbo commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1274181193
Yes, you can do it :) @tanruixiang
--
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] caipengbo commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r974820106
##########
src/redis_disk.h:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <string>
+#include "redis_db.h"
+#include "redis_metadata.h"
+
+namespace Redis {
+
+class Disk : public Database {
+ public:
+ explicit Disk(Engine::Storage *storage, const std::string &ns): Database(storage, ns) {
+ this->option_.include_memtabtles = true;
+ this->option_.include_files = true;
+ }
+ rocksdb::Status GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft = Slice(),
+ Slice subkeyright = Slice());
+ rocksdb::Status GetStringSize(const Slice &user_key, uint64_t *key_size);
Review Comment:
Indeed, we need to create different objects. We added an additional command to calculate the size of different types, and your implementation is also a good method.
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973582104
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft ,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(this->option, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetStringSize(const Slice &user_key, uint64_t *key_size) {
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key + static_cast<char>(0)));
+ return db_->GetApproximateSizes(this->option, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+
+rocksdb::Status Disk::GetSetSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetListSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ ListMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisList, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string buf;
+ PutFixed64(&buf, metadata.head);
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size, buf);
+}
+
+rocksdb::Status Disk::GetZsetSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ ZSetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisZSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string score_bytes;
+ PutDouble(&score_bytes, kMinScore);
+ s = this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kZSetScoreColumnFamilyName),
+ key_size, score_bytes, score_bytes);
+ if (!s.ok()) return s;
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetBitmapSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ BitmapMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisBitmap, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string score_bytes;
+ PutDouble(&score_bytes, kMinScore);
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size, std::to_string(0), std::to_string(0));
+}
+
+rocksdb::Status Disk::GetSortedintSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ SortedintMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisSortedint, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string prefix_key, next_version_prefix_key, start_buf;
+ PutFixed64(&start_buf, 0);
+ return this->GetApproximateSizes(metadata, ns_key,
Review Comment:
You can omit `this` when calling class functions here and in other functions as well.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973601081
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ rocksdb::Status s;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ s = GetStringSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisHash:
+ s = GetHashSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisBitmap:
+ s = GetBitmapSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisList:
+ s = GetListSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSet:
+ s = GetSetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSortedint:
+ s = GetSortedintSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisZSet:
+ s = GetZsetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
+ }
+ return s;
+}
+rocksdb::Status Disk::GetStringSize(const Slice &ns_key, uint64_t *key_size) {
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key.ToString() + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &ns_key, uint64_t *key_size) {
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+
+rocksdb::Status Disk::GetSetSize(const Slice &ns_key, uint64_t *key_size) {
+ SetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetListSize(const Slice &ns_key, uint64_t *key_size) {
+ ListMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisList, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string buf;
+ PutFixed64(&buf, metadata.head);
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size, buf);
+}
+
+rocksdb::Status Disk::GetZsetSize(const Slice &ns_key, uint64_t *key_size) {
+ ZSetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisZSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string score_bytes;
+ PutDouble(&score_bytes, kMinScore);
+ s = GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kZSetScoreColumnFamilyName),
+ key_size, score_bytes, score_bytes);
+ if (!s.ok()) return s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetBitmapSize(const Slice &ns_key, uint64_t *key_size) {
+ BitmapMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisBitmap, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Review Comment:
As in https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973585761,
you can put them out of the `switch`, then it can be eliminated from every method in this 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] torwig commented on pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1264705355
@tanruixiang Hi, I've done some tests locally. For example, if I set `x` to a string of length 50 and then use `disk` command - I get something about 80 as a result. Then I restart `kvrocks` and repeat `disk` command. And this time the result is something about 1500. Is it some kind of approximation by the `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] git-hulk commented on pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1272282126
> > Sure, I have read through the implementation, and overall is good to me. I'm still wondering if we have a more general way to reduce duplicate codes, I will approve this PR soon if can't figure out a better way.
>
> Do you have any ideas? It seems like it should be impossible to reduce each type of custom operation.
The main issue is `GetMetadata` requires to pass the type, so another way is to allow skipping type check in `GetMetadata`, but current implementation is also fine to me. To see others have thoughts on this cc @ShooterIT @PragmaTwice @caipengbo
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985334098
##########
tests/gocase/unit/disk/disk_test.go:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package disk
+
+import (
+ "context"
+ "strconv"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func TestDisk(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{
+ "rocksdb.compression": "no",
+ "rocksdb.write_buffer_size": "1",
+ })
+ defer srv.Close()
+ ctx := context.Background()
+ rdb := srv.NewClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+ estimationFactor := 0.5
+ t.Run("Disk usage String", func(t *testing.T) {
+ require.NoError(t, rdb.Set(ctx, "stringkey", "aaaaaaaaaaaaaaaaaaaaaaaa", 0).Err())
+ val, err := rdb.Do(ctx, "Disk", "usage", "stringkey").Int()
+ require.NoError(t, err)
+ require.GreaterOrEqual(t, val, 1)
Review Comment:
> IMHO, the expected value is weird. And I wonder if it can be a range rather than only a lower bound.
sounds good. Let me try.
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r976073175
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ return GetStringSize(ns_key, key_size);
+ case RedisType::kRedisHash:
+ return GetHashSize(ns_key, key_size);
+ case RedisType::kRedisBitmap:
+ return GetBitmapSize(ns_key, key_size);
+ case RedisType::kRedisList:
+ return GetListSize(ns_key, key_size);
+ case RedisType::kRedisSet:
+ return GetSetSize(ns_key, key_size);
+ case RedisType::kRedisSortedint:
+ return GetSortedintSize(ns_key, key_size);
+ case RedisType::kRedisZSet:
+ return GetZsetSize(ns_key, key_size);
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
Review Comment:
Yes, I can add the design of the Stream data type to that document. I just thought that that document can be updated with the changes that are released. And right now the Stream type isn't released. @git-hulk @PragmaTwice Am I right?
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r976077131
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ return GetStringSize(ns_key, key_size);
+ case RedisType::kRedisHash:
+ return GetHashSize(ns_key, key_size);
+ case RedisType::kRedisBitmap:
+ return GetBitmapSize(ns_key, key_size);
+ case RedisType::kRedisList:
+ return GetListSize(ns_key, key_size);
+ case RedisType::kRedisSet:
+ return GetSetSize(ns_key, key_size);
+ case RedisType::kRedisSortedint:
+ return GetSortedintSize(ns_key, key_size);
+ case RedisType::kRedisZSet:
+ return GetZsetSize(ns_key, key_size);
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
Review Comment:
I see, then I will update the design docs.
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973584783
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft ,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(this->option, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetStringSize(const Slice &user_key, uint64_t *key_size) {
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key + static_cast<char>(0)));
+ return db_->GetApproximateSizes(this->option, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+
+rocksdb::Status Disk::GetSetSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetListSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ ListMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisList, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string buf;
+ PutFixed64(&buf, metadata.head);
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size, buf);
+}
+
+rocksdb::Status Disk::GetZsetSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
Review Comment:
> To be consistent with the rest of Kvrock's code, please use the indentation of 2 spaces instead of 4.
Thank you. It has been 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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973594822
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ rocksdb::Status s;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ s = GetStringSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisHash:
+ s = GetHashSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisBitmap:
+ s = GetBitmapSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisList:
+ s = GetListSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSet:
+ s = GetSetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSortedint:
+ s = GetSortedintSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisZSet:
+ s = GetZsetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisNone:
+ s = GetHashSize(ns_key, key_size);
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ break;
Review Comment:
I think you don't need `break` here because of `return`.
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973593653
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <gtest/gtest.h>
+#include <memory>
+#include <vector>
+#include <thread>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ }
+ ~RedisDiskTest() = default;
+
+protected:
+};
+
+TEST_F(RedisDiskTest, StringDisk) {
+ key_ = "stringdisk_key";
+ std::unique_ptr<Redis::String> string = Util::MakeUnique<Redis::String>(storage_, "disk_ns_string");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_string");
+ std::vector<int> value_size{1, 1024};
+ for(auto &p : value_size){
+ EXPECT_TRUE(string->Set(key_, std::string(p, 'a')).ok());
+ std::string got_value;
+ EXPECT_TRUE(string->Get(key_, &got_value).ok());
+ EXPECT_EQ(got_value, std::string(p, 'a'));
+ std::string value;
+ string->Get(key_, &value);
+ uint64_t result = 0;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisString, &result).ok());
+ EXPECT_GE(result, p);
+ break;
+ }
+ string->Del(key_);
+}
+
+TEST_F(RedisDiskTest, HashDisk) {
+ std::unique_ptr<Redis::Hash> hash = Util::MakeUnique<Redis::Hash>(storage_, "disk_ns_hash");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_hash");
+ key_ = "hashdisk_key";
+ fields_ = {"hashdisk_kkey1", "hashdisk_kkey2"};
+ values_.resize(2);
+ std::vector<int>value_size{1, 1024};
+ for(int i = 0 ;i < int(fields_.size()); i++){
+ values_[i] = std::string(value_size[i],'a');
+ }
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(fields_.size()); i++) {
+ sum += uint64_t(fields_[i].size()) + values_[i].size();
+ rocksdb::Status s = hash->Set(key_, fields_[i], values_[i], &ret);
+ EXPECT_TRUE(s.ok() && ret == 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisHash, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ hash->Del(key_);
+}
+
+TEST_F(RedisDiskTest, SetDisk) {
+ std::unique_ptr<Redis::Set> set = Util::MakeUnique<Redis::Set>(storage_, "disk_ns_set");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_set");
+ key_ = "setdisk_key";
+ values_.resize(2);
+ std::vector<int>value_size{1, 1024};
+ for(int i = 0;i < int(values_.size()); i++){
+ values_[i] = std::string(value_size[i],'a');
+ }
+
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(values_.size()); i++) {
+ std::vector<Slice>tmpv{values_[i]};
+ sum += values_[i].size();
+ rocksdb::Status s = set->Add(key_, tmpv, &ret);
+ EXPECT_TRUE(s.ok() && ret == 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisSet, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ set->Del(key_);
+}
+
+
+TEST_F(RedisDiskTest, ListDisk) {
+ std::unique_ptr<Redis::List> list = Util::MakeUnique<Redis::List>(storage_, "disk_ns_list");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_list");
+ key_ = "listdisk_key";
+ values_.resize(2);
+ std::vector<int>value_size{1,1024};
+ for(int i = 0;i < int(values_.size()); i++){
+ values_[i] = std::string(value_size[i],'a');
+ }
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(values_.size()); i++) {
+ std::vector<Slice>tmpv{values_[i]};
+ sum += values_[i].size();
+ rocksdb::Status s = list->Push(key_, tmpv, false, &ret);
+ EXPECT_TRUE(s.ok() && ret == i + 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisList, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ list->Del(key_);
+}
+
+TEST_F(RedisDiskTest, ZsetDisk) {
+ std::unique_ptr<Redis::ZSet> zset = Util::MakeUnique<Redis::ZSet>(storage_, "disk_ns_zet");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_zet");
+ key_ = "zsetdisk_key";
+ std::vector<MemberScore> mscores(2);
+ std::vector<int>value_size{1,1024};
+ for(int i = 0;i < int(value_size.size()); i++){
+ mscores[i].member = std::string(value_size[i],'a');
+ mscores[i].score = 1.0 * value_size[int(values_.size()) - i - 1];
+ }
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(mscores.size()); i++) {
+ std::vector<MemberScore> tmpv{mscores[i]};
+ sum += 2 * mscores[i].member.size();
+ rocksdb::Status s = zset->Add(key_, 0, &tmpv, &ret);
+ EXPECT_TRUE(s.ok() && ret == 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisZSet, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ zset->Del(key_);
+}
+
+TEST_F(RedisDiskTest, BitmapDisk) {
+ std::unique_ptr<Redis::Bitmap> bitmap = Util::MakeUnique<Redis::Bitmap>(storage_, "disk_ns_bitmap");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_bitmap");
+ key_ = "bitmapdisk_key";
+ uint32_t offsets[] = {0, 123, 1024*8, 1024*8+1, 3*1024*8, 3*1024*8+1};
+
+ for (int i = 0; i < 6 ;i++) {
+ bool bit = false;
+ bitmap->GetBit(key_, offsets[i], &bit);
+ EXPECT_FALSE(bit);
+ bitmap->SetBit(key_, offsets[i], true, &bit);
+ bitmap->GetBit(key_, offsets[i], &bit);
+ EXPECT_TRUE(bit);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisBitmap, &key_size).ok());
+ EXPECT_GE(key_size, i + 1);
+ }
+ bitmap->Del(key_);
+}
+
+TEST_F(RedisDiskTest, SortedintDisk) {
+ std::unique_ptr<Redis::Sortedint> sortedint = Util::MakeUnique<Redis::Sortedint>(storage_, "disk_ns_sortedint");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_sortedint");
+ key_ = "sortedintdisk_key";
+ int ret;
+ for(int i=0;i<10;i++){
Review Comment:
Just formatting (add spaces like on line 174).
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973593536
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <gtest/gtest.h>
+#include <memory>
+#include <vector>
+#include <thread>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ }
+ ~RedisDiskTest() = default;
+
+protected:
+};
+
+TEST_F(RedisDiskTest, StringDisk) {
+ key_ = "stringdisk_key";
+ std::unique_ptr<Redis::String> string = Util::MakeUnique<Redis::String>(storage_, "disk_ns_string");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_string");
+ std::vector<int> value_size{1, 1024};
+ for(auto &p : value_size){
+ EXPECT_TRUE(string->Set(key_, std::string(p, 'a')).ok());
+ std::string got_value;
+ EXPECT_TRUE(string->Get(key_, &got_value).ok());
+ EXPECT_EQ(got_value, std::string(p, 'a'));
+ std::string value;
+ string->Get(key_, &value);
+ uint64_t result = 0;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisString, &result).ok());
+ EXPECT_GE(result, p);
+ break;
Review Comment:
I'm wondering why this operator (`break`) is here. With it, the loop will run only one time. But if you want to do something just one time, there is no reason to introduce the loop.
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973601406
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ rocksdb::Status s;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ s = GetStringSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisHash:
+ s = GetHashSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisBitmap:
+ s = GetBitmapSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisList:
+ s = GetListSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSet:
+ s = GetSetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSortedint:
+ s = GetSortedintSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisZSet:
+ s = GetZsetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
+ }
+ return s;
+}
+rocksdb::Status Disk::GetStringSize(const Slice &ns_key, uint64_t *key_size) {
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key.ToString() + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &ns_key, uint64_t *key_size) {
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+
+rocksdb::Status Disk::GetSetSize(const Slice &ns_key, uint64_t *key_size) {
+ SetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetListSize(const Slice &ns_key, uint64_t *key_size) {
+ ListMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisList, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string buf;
+ PutFixed64(&buf, metadata.head);
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size, buf);
+}
+
+rocksdb::Status Disk::GetZsetSize(const Slice &ns_key, uint64_t *key_size) {
+ ZSetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisZSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string score_bytes;
+ PutDouble(&score_bytes, kMinScore);
+ s = GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kZSetScoreColumnFamilyName),
+ key_size, score_bytes, score_bytes);
+ if (!s.ok()) return s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetBitmapSize(const Slice &ns_key, uint64_t *key_size) {
+ BitmapMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisBitmap, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Review Comment:
We need to set different types of metadata variables. The code seems to be more unreadable if the Use inheritance mechanism 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] tanruixiang commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973584619
##########
src/redis_disk.h:
##########
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <string>
+#include "redis_db.h"
+#include "redis_metadata.h"
+
+namespace Redis {
+
+class Disk : public Database {
+ public:
+ explicit Disk(Engine::Storage *storage, const std::string &ns): Database(storage, ns) {
+ this->option.include_memtabtles = true;
+ this->option.include_files = true;
+ }
+ rocksdb::Status GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft = Slice(),
+ Slice subkeyright = Slice());
+ rocksdb::Status GetStringSize(const Slice &user_key, uint64_t *key_size);
+ rocksdb::Status GetHashSize(const Slice &user_key, uint64_t *key_size);
+ rocksdb::Status GetSetSize(const Slice &user_key, uint64_t *key_size);
+ rocksdb::Status GetListSize(const Slice &user_key, uint64_t *key_size);
+ rocksdb::Status GetZsetSize(const Slice &user_key, uint64_t *key_size);
+ rocksdb::Status GetBitmapSize(const Slice &user_key, uint64_t *key_size);
+ rocksdb::Status GetSortedintSize(const Slice &user_key, uint64_t *key_size);
+ private:
+ rocksdb::SizeApproximationOptions option;
Review Comment:
> Across Kvrocks codebase private members of the class are named with the `_` suffix. So you can rename `option` to `option` and omit using `this->option` everywhere.
thank you. It has been modified.
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft ,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(this->option, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetStringSize(const Slice &user_key, uint64_t *key_size) {
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key + static_cast<char>(0)));
+ return db_->GetApproximateSizes(this->option, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+
+rocksdb::Status Disk::GetSetSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetListSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ ListMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisList, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string buf;
+ PutFixed64(&buf, metadata.head);
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size, buf);
+}
+
+rocksdb::Status Disk::GetZsetSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ ZSetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisZSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string score_bytes;
+ PutDouble(&score_bytes, kMinScore);
+ s = this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kZSetScoreColumnFamilyName),
+ key_size, score_bytes, score_bytes);
+ if (!s.ok()) return s;
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetBitmapSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ BitmapMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisBitmap, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string score_bytes;
+ PutDouble(&score_bytes, kMinScore);
+ return this->GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size, std::to_string(0), std::to_string(0));
+}
+
+rocksdb::Status Disk::GetSortedintSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ SortedintMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisSortedint, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string prefix_key, next_version_prefix_key, start_buf;
+ PutFixed64(&start_buf, 0);
+ return this->GetApproximateSizes(metadata, ns_key,
Review Comment:
> You can omit `this` when calling class functions here and in other functions as well.
thank you. It has been 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] git-hulk commented on pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1274175926
Yes, @caipengbo you can help to update as well when you're free. https://github.com/apache/incubator-kvrocks-website/blob/main/docs/01-supported-commands.md
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk merged PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r976074755
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ return GetStringSize(ns_key, key_size);
+ case RedisType::kRedisHash:
+ return GetHashSize(ns_key, key_size);
+ case RedisType::kRedisBitmap:
+ return GetBitmapSize(ns_key, key_size);
+ case RedisType::kRedisList:
+ return GetListSize(ns_key, key_size);
+ case RedisType::kRedisSet:
+ return GetSetSize(ns_key, key_size);
+ case RedisType::kRedisSortedint:
+ return GetSortedintSize(ns_key, key_size);
+ case RedisType::kRedisZSet:
+ return GetZsetSize(ns_key, key_size);
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
Review Comment:
Sure, I had updated the supported command in the Kvrocks website: https://github.com/apache/incubator-kvrocks-website/blob/main/docs/01-supported-commands.md
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1258977424
@git-hulk I think I already know why. According to [this](https://github.com/facebook/rocksdb/issues/10733), to prevent the situation of 0 size during the test, it is necessary to save the data in different data blocks of sst as much as possible. This requires us to enter a lot of data when testing.
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985057189
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <memory>
+#include <string>
+#include <vector>
+#include <gtest/gtest.h>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+#include "redis_stream.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ delete storage_;
Review Comment:
Have you tried to use this code without deleting and creating new storage? `config_` is passed through the pointer and it is used mainly within the `Open()` call.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1272273276
> Sure, I have read through the implementation, and overall is good to me. I'm still wondering if we have a more general way to reduce duplicate codes, I will approve this PR soon if can't figure out a better way.
Do you have any ideas? It seems like it should be impossible to reduce each type of custom operation.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1271705393
> @git-hulk @torwig @caipengbo Hi. Do you have time to review?
Sure, I have read through the implementation, and overall is good to me. I'm still wondering if we have a more general way to reduce duplicate codes.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1274161866
Thanks for @tanruixiang contribution and patient, also thanks to everyone who reviewed this PR. Merging...
--
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] caipengbo commented on pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
caipengbo commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1274172976
We also need to update our website :) @git-hulk
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985056631
##########
src/redis_disk.h:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <string>
+#include "redis_db.h"
+#include "redis_metadata.h"
+
+namespace Redis {
+
+class Disk : public Database {
+ public:
+ explicit Disk(Engine::Storage *storage, const std::string &ns): Database(storage, ns) {
+ this->option_.include_memtabtles = true;
+ this->option_.include_files = true;
+ }
+ rocksdb::Status GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft = Slice(),
+ Slice subkeyright = Slice());
+ rocksdb::Status GetStringSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetHashSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetSetSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetListSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetZsetSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetBitmapSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetSortedintSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetStreamSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size);
+ private:
+ rocksdb::SizeApproximationOptions option_;
Review Comment:
@tanruixiang Hi, I'll try to review it later today.
After a brisk view:
- fix indentation here
- keep one blank line between method definitions in the .cc file
- using `this` on line 32, 33 is redundant
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985057674
##########
src/redis_disk.h:
##########
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <string>
+#include "redis_db.h"
+#include "redis_metadata.h"
+
+namespace Redis {
+
+class Disk : public Database {
+ public:
+ explicit Disk(Engine::Storage *storage, const std::string &ns): Database(storage, ns) {
+ this->option_.include_memtabtles = true;
+ this->option_.include_files = true;
+ }
+ rocksdb::Status GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft = Slice(),
+ Slice subkeyright = Slice());
+ rocksdb::Status GetStringSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetHashSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetSetSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetListSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetZsetSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetBitmapSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetSortedintSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetStreamSize(const Slice &ns_key, uint64_t *key_size);
+ rocksdb::Status GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size);
+ private:
+ rocksdb::SizeApproximationOptions option_;
Review Comment:
Thanks a lot, it has been 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] caipengbo commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973893877
##########
src/redis_disk.h:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <string>
+#include "redis_db.h"
+#include "redis_metadata.h"
+
+namespace Redis {
+
+class Disk : public Database {
+ public:
+ explicit Disk(Engine::Storage *storage, const std::string &ns): Database(storage, ns) {
+ this->option_.include_memtabtles = true;
+ this->option_.include_files = true;
+ }
+ rocksdb::Status GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft = Slice(),
+ Slice subkeyright = Slice());
+ rocksdb::Status GetStringSize(const Slice &user_key, uint64_t *key_size);
Review Comment:
Should we put these functions in each type of files (Like `redis_string.h` and `redis_string.cpp`)?
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1250324722
Is it possible that it is a bug of rocksdb? I can get the value of the corresponding key, but the size occupied by the key is 0. And this problem occurs occasionally.
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973584932
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft ,
Review Comment:
My bad, my apologies.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973585761
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetStringSize(const Slice &user_key, uint64_t *key_size) {
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Review Comment:
These code appears in almost every method in this file, so I think you can just define something like
```
rocksdb::Status GetHashSize(auto ns_key, auto metadata, auto key_size) // fill these types
```
and then
```
RedisType type = ...;
std::string ns_key;
AppendNamespacePrefix(user_key, &ns_key);
Metadata metadata(type, false);
rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
switch (redisType) {
case kRedisHash: GetHashSize(...); break;
case kRedisSomething: GetSomethingSize(...) ...
}
```
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetStringSize(const Slice &user_key, uint64_t *key_size) {
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Review Comment:
These code appears in almost every method in this file, so I think you can just define something like
```
rocksdb::Status GetHashSize(auto ns_key, auto metadata, auto key_size) // fill these types
```
and then
```
RedisType type = ...;
std::string ns_key;
AppendNamespacePrefix(user_key, &ns_key);
Metadata metadata(type, false);
rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
switch (redisType) {
case kRedisHash: GetHashSize(...); break;
case kRedisSomething: GetSomethingSize(...)
...
}
```
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetStringSize(const Slice &user_key, uint64_t *key_size) {
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Review Comment:
These code appears in almost every method in this file, so I think you can just define something like
```
rocksdb::Status GetHashSize(auto ns_key, auto metadata, auto key_size) // fill these types
```
and then
```
RedisType type = ...;
std::string ns_key;
AppendNamespacePrefix(user_key, &ns_key);
Metadata metadata(type, false);
rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
switch (redisType) {
case kRedisHash: GetHashSize(...); break;
case kRedisSomething: GetSomethingSize(...)
...
}
```
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973585262
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft ,
Review Comment:
> My bad, my apologies.
Don't apologize. Thank you very much for your help. 😁
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1256375626
> Sorry for missing this comment, I have no enough time to inspect this case yet.
@git-hulk Do not worry. I've been looking at this question. I have confirmed that there should be no problem with the source code of `rocksdb`. I found a strange phenomenon, if you get the corresponding key directly, you can find the value, but calling GetApproximateSizes will get 0.
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r976037522
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ return GetStringSize(ns_key, key_size);
+ case RedisType::kRedisHash:
+ return GetHashSize(ns_key, key_size);
+ case RedisType::kRedisBitmap:
+ return GetBitmapSize(ns_key, key_size);
+ case RedisType::kRedisList:
+ return GetListSize(ns_key, key_size);
+ case RedisType::kRedisSet:
+ return GetSetSize(ns_key, key_size);
+ case RedisType::kRedisSortedint:
+ return GetSortedintSize(ns_key, key_size);
+ case RedisType::kRedisZSet:
+ return GetZsetSize(ns_key, key_size);
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
Review Comment:
@torwig I'm sorry for missing the `stream`. I added the corresponding type based on [design document](https://kvrocks.apache.org/docs/Design/design-structure-on-rocksdb). There is no documentation for `stream` here. I noticed that `stream` are implemented by you, do you have time to add `stream` into [this website](https://kvrocks.apache.org/docs/Design/design-structure-on-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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973584739
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft ,
Review Comment:
>
> I'd recommend making the `key_size` argument the last one because it is the output one and it is passed by a pointer.
Hi. The last two are parameters with default values. In fact, `key_size` are the last parameters without default values.
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973929751
##########
src/redis_disk.h:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <string>
+#include "redis_db.h"
+#include "redis_metadata.h"
+
+namespace Redis {
+
+class Disk : public Database {
+ public:
+ explicit Disk(Engine::Storage *storage, const std::string &ns): Database(storage, ns) {
+ this->option_.include_memtabtles = true;
+ this->option_.include_files = true;
+ }
+ rocksdb::Status GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft = Slice(),
+ Slice subkeyright = Slice());
+ rocksdb::Status GetStringSize(const Slice &user_key, uint64_t *key_size);
Review Comment:
@caipengbo Do you mean that each type should implement its own function (e.g. `GetApproximateKeySize`) ? Otherwise, I think, it's not the best idea to spread functions of `Disk` class among different files (one function per 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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973576361
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,166 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
Review Comment:
Hi @tanruixiang , thanks for you contribution!
I noticed that there are many duplicate code snippets in this file, I think you can think about how to reduce/eliminate them.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973602136
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ rocksdb::Status s;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ s = GetStringSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisHash:
+ s = GetHashSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisBitmap:
+ s = GetBitmapSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisList:
+ s = GetListSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSet:
+ s = GetSetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSortedint:
+ s = GetSortedintSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisZSet:
+ s = GetZsetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
+ }
+ return s;
+}
+rocksdb::Status Disk::GetStringSize(const Slice &ns_key, uint64_t *key_size) {
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key.ToString() + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &ns_key, uint64_t *key_size) {
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+
+rocksdb::Status Disk::GetSetSize(const Slice &ns_key, uint64_t *key_size) {
+ SetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetListSize(const Slice &ns_key, uint64_t *key_size) {
+ ListMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisList, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string buf;
+ PutFixed64(&buf, metadata.head);
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size, buf);
+}
+
+rocksdb::Status Disk::GetZsetSize(const Slice &ns_key, uint64_t *key_size) {
+ ZSetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisZSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string score_bytes;
+ PutDouble(&score_bytes, kMinScore);
+ s = GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kZSetScoreColumnFamilyName),
+ key_size, score_bytes, score_bytes);
+ if (!s.ok()) return s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetBitmapSize(const Slice &ns_key, uint64_t *key_size) {
+ BitmapMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisBitmap, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Review Comment:
Oh you are right, a factory function is needed for `Metadata`. So weird since the ctor of `Metadata` is public.
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973586605
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetStringSize(const Slice &user_key, uint64_t *key_size) {
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Review Comment:
> Or you can even do not define one method per redis type, I think it is ok to just implement it in the switch clause.
Thank you. I'm doing it now.
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973593959
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <gtest/gtest.h>
+#include <memory>
+#include <vector>
+#include <thread>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ }
+ ~RedisDiskTest() = default;
+
+protected:
+};
+
+TEST_F(RedisDiskTest, StringDisk) {
+ key_ = "stringdisk_key";
+ std::unique_ptr<Redis::String> string = Util::MakeUnique<Redis::String>(storage_, "disk_ns_string");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_string");
+ std::vector<int> value_size{1, 1024};
+ for(auto &p : value_size){
+ EXPECT_TRUE(string->Set(key_, std::string(p, 'a')).ok());
+ std::string got_value;
+ EXPECT_TRUE(string->Get(key_, &got_value).ok());
+ EXPECT_EQ(got_value, std::string(p, 'a'));
+ std::string value;
+ string->Get(key_, &value);
+ uint64_t result = 0;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisString, &result).ok());
+ EXPECT_GE(result, p);
+ break;
+ }
+ string->Del(key_);
+}
+
+TEST_F(RedisDiskTest, HashDisk) {
+ std::unique_ptr<Redis::Hash> hash = Util::MakeUnique<Redis::Hash>(storage_, "disk_ns_hash");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_hash");
+ key_ = "hashdisk_key";
+ fields_ = {"hashdisk_kkey1", "hashdisk_kkey2"};
+ values_.resize(2);
+ std::vector<int>value_size{1, 1024};
+ for(int i = 0 ;i < int(fields_.size()); i++){
+ values_[i] = std::string(value_size[i],'a');
+ }
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(fields_.size()); i++) {
+ sum += uint64_t(fields_[i].size()) + values_[i].size();
+ rocksdb::Status s = hash->Set(key_, fields_[i], values_[i], &ret);
+ EXPECT_TRUE(s.ok() && ret == 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisHash, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ hash->Del(key_);
+}
+
+TEST_F(RedisDiskTest, SetDisk) {
+ std::unique_ptr<Redis::Set> set = Util::MakeUnique<Redis::Set>(storage_, "disk_ns_set");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_set");
+ key_ = "setdisk_key";
+ values_.resize(2);
+ std::vector<int>value_size{1, 1024};
+ for(int i = 0;i < int(values_.size()); i++){
+ values_[i] = std::string(value_size[i],'a');
+ }
+
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(values_.size()); i++) {
+ std::vector<Slice>tmpv{values_[i]};
+ sum += values_[i].size();
+ rocksdb::Status s = set->Add(key_, tmpv, &ret);
+ EXPECT_TRUE(s.ok() && ret == 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisSet, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ set->Del(key_);
+}
+
+
+TEST_F(RedisDiskTest, ListDisk) {
+ std::unique_ptr<Redis::List> list = Util::MakeUnique<Redis::List>(storage_, "disk_ns_list");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_list");
+ key_ = "listdisk_key";
+ values_.resize(2);
+ std::vector<int>value_size{1,1024};
+ for(int i = 0;i < int(values_.size()); i++){
+ values_[i] = std::string(value_size[i],'a');
+ }
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(values_.size()); i++) {
+ std::vector<Slice>tmpv{values_[i]};
+ sum += values_[i].size();
+ rocksdb::Status s = list->Push(key_, tmpv, false, &ret);
+ EXPECT_TRUE(s.ok() && ret == i + 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisList, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ list->Del(key_);
+}
+
+TEST_F(RedisDiskTest, ZsetDisk) {
+ std::unique_ptr<Redis::ZSet> zset = Util::MakeUnique<Redis::ZSet>(storage_, "disk_ns_zet");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_zet");
+ key_ = "zsetdisk_key";
+ std::vector<MemberScore> mscores(2);
+ std::vector<int>value_size{1,1024};
+ for(int i = 0;i < int(value_size.size()); i++){
+ mscores[i].member = std::string(value_size[i],'a');
+ mscores[i].score = 1.0 * value_size[int(values_.size()) - i - 1];
+ }
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(mscores.size()); i++) {
+ std::vector<MemberScore> tmpv{mscores[i]};
+ sum += 2 * mscores[i].member.size();
+ rocksdb::Status s = zset->Add(key_, 0, &tmpv, &ret);
+ EXPECT_TRUE(s.ok() && ret == 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisZSet, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ zset->Del(key_);
+}
+
+TEST_F(RedisDiskTest, BitmapDisk) {
+ std::unique_ptr<Redis::Bitmap> bitmap = Util::MakeUnique<Redis::Bitmap>(storage_, "disk_ns_bitmap");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_bitmap");
+ key_ = "bitmapdisk_key";
+ uint32_t offsets[] = {0, 123, 1024*8, 1024*8+1, 3*1024*8, 3*1024*8+1};
+
+ for (int i = 0; i < 6 ;i++) {
+ bool bit = false;
+ bitmap->GetBit(key_, offsets[i], &bit);
+ EXPECT_FALSE(bit);
+ bitmap->SetBit(key_, offsets[i], true, &bit);
+ bitmap->GetBit(key_, offsets[i], &bit);
+ EXPECT_TRUE(bit);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisBitmap, &key_size).ok());
+ EXPECT_GE(key_size, i + 1);
+ }
+ bitmap->Del(key_);
+}
+
+TEST_F(RedisDiskTest, SortedintDisk) {
+ std::unique_ptr<Redis::Sortedint> sortedint = Util::MakeUnique<Redis::Sortedint>(storage_, "disk_ns_sortedint");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_sortedint");
+ key_ = "sortedintdisk_key";
+ int ret;
+ for(int i=0;i<10;i++){
Review Comment:
I noticed that the running results of ci are somewhat different from those of my native machine. At the same time, because this is a draft pr, I am using ci for testing.I found that a byte of data is often not detected due to the problem with the settings of Writebatch(sync is default false).
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973594201
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,163 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ rocksdb::Status s;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ s = GetStringSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisHash:
+ s = GetHashSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisBitmap:
+ s = GetBitmapSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisList:
+ s = GetListSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSet:
+ s = GetSetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisSortedint:
+ s = GetSortedintSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisZSet:
+ s = GetZsetSize(ns_key, key_size);
+ break;
+ case RedisType::kRedisNone:
+ s = GetHashSize(ns_key, key_size);
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ break;
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
+ break;
+ }
+ return s;
+}
+rocksdb::Status Disk::GetStringSize(const Slice &ns_key, uint64_t *key_size) {
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key.ToString() + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &ns_key, uint64_t *key_size) {
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+
+rocksdb::Status Disk::GetSetSize(const Slice &ns_key, uint64_t *key_size) {
+ SetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetListSize(const Slice &ns_key, uint64_t *key_size) {
+ ListMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisList, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string buf;
+ PutFixed64(&buf, metadata.head);
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size, buf);
+}
+
+rocksdb::Status Disk::GetZsetSize(const Slice &ns_key, uint64_t *key_size) {
+ ZSetMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisZSet, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string score_bytes;
+ PutDouble(&score_bytes, kMinScore);
+ s = GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kZSetScoreColumnFamilyName),
+ key_size, score_bytes, score_bytes);
+ if (!s.ok()) return s;
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size);
+}
+
+rocksdb::Status Disk::GetBitmapSize(const Slice &ns_key, uint64_t *key_size) {
+ BitmapMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisBitmap, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string score_bytes;
+ PutDouble(&score_bytes, kMinScore);
+ return GetApproximateSizes(metadata, ns_key,
+ storage_->GetCFHandle(Engine::kSubkeyColumnFamilyName),
+ key_size, std::to_string(0), std::to_string(0));
+}
+
+rocksdb::Status Disk::GetSortedintSize(const Slice &ns_key, uint64_t *key_size) {
+ SortedintMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisSortedint, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ std::string prefix_key, next_version_prefix_key, start_buf;
Review Comment:
`prefix_key` and `next_version_prefix_key` variables are declared but not 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] tanruixiang commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973594178
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <gtest/gtest.h>
Review Comment:
> It's not a big deal, but I'd recommend grouping headers of the standard library together (`chrono` after third-party `gtest` header)
Ok. Very good advice.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1258984055
> @git-hulk I think I already know why. According to [this](https://github.com/facebook/rocksdb/issues/10733), to prevent the situation of 0 size during the test, it is necessary to save the data in different data blocks of sst as much as possible. This requires us to enter a lot of data when testing.
Nice!
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985058276
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <memory>
+#include <string>
+#include <vector>
+#include <gtest/gtest.h>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+#include "redis_stream.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ delete storage_;
Review Comment:
Let me try 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] git-hulk commented on pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1274179887
Yes, GitHub actions will deploy the update automatically.
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973593959
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <gtest/gtest.h>
+#include <memory>
+#include <vector>
+#include <thread>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ }
+ ~RedisDiskTest() = default;
+
+protected:
+};
+
+TEST_F(RedisDiskTest, StringDisk) {
+ key_ = "stringdisk_key";
+ std::unique_ptr<Redis::String> string = Util::MakeUnique<Redis::String>(storage_, "disk_ns_string");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_string");
+ std::vector<int> value_size{1, 1024};
+ for(auto &p : value_size){
+ EXPECT_TRUE(string->Set(key_, std::string(p, 'a')).ok());
+ std::string got_value;
+ EXPECT_TRUE(string->Get(key_, &got_value).ok());
+ EXPECT_EQ(got_value, std::string(p, 'a'));
+ std::string value;
+ string->Get(key_, &value);
+ uint64_t result = 0;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisString, &result).ok());
+ EXPECT_GE(result, p);
+ break;
+ }
+ string->Del(key_);
+}
+
+TEST_F(RedisDiskTest, HashDisk) {
+ std::unique_ptr<Redis::Hash> hash = Util::MakeUnique<Redis::Hash>(storage_, "disk_ns_hash");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_hash");
+ key_ = "hashdisk_key";
+ fields_ = {"hashdisk_kkey1", "hashdisk_kkey2"};
+ values_.resize(2);
+ std::vector<int>value_size{1, 1024};
+ for(int i = 0 ;i < int(fields_.size()); i++){
+ values_[i] = std::string(value_size[i],'a');
+ }
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(fields_.size()); i++) {
+ sum += uint64_t(fields_[i].size()) + values_[i].size();
+ rocksdb::Status s = hash->Set(key_, fields_[i], values_[i], &ret);
+ EXPECT_TRUE(s.ok() && ret == 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisHash, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ hash->Del(key_);
+}
+
+TEST_F(RedisDiskTest, SetDisk) {
+ std::unique_ptr<Redis::Set> set = Util::MakeUnique<Redis::Set>(storage_, "disk_ns_set");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_set");
+ key_ = "setdisk_key";
+ values_.resize(2);
+ std::vector<int>value_size{1, 1024};
+ for(int i = 0;i < int(values_.size()); i++){
+ values_[i] = std::string(value_size[i],'a');
+ }
+
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(values_.size()); i++) {
+ std::vector<Slice>tmpv{values_[i]};
+ sum += values_[i].size();
+ rocksdb::Status s = set->Add(key_, tmpv, &ret);
+ EXPECT_TRUE(s.ok() && ret == 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisSet, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ set->Del(key_);
+}
+
+
+TEST_F(RedisDiskTest, ListDisk) {
+ std::unique_ptr<Redis::List> list = Util::MakeUnique<Redis::List>(storage_, "disk_ns_list");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_list");
+ key_ = "listdisk_key";
+ values_.resize(2);
+ std::vector<int>value_size{1,1024};
+ for(int i = 0;i < int(values_.size()); i++){
+ values_[i] = std::string(value_size[i],'a');
+ }
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(values_.size()); i++) {
+ std::vector<Slice>tmpv{values_[i]};
+ sum += values_[i].size();
+ rocksdb::Status s = list->Push(key_, tmpv, false, &ret);
+ EXPECT_TRUE(s.ok() && ret == i + 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisList, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ list->Del(key_);
+}
+
+TEST_F(RedisDiskTest, ZsetDisk) {
+ std::unique_ptr<Redis::ZSet> zset = Util::MakeUnique<Redis::ZSet>(storage_, "disk_ns_zet");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_zet");
+ key_ = "zsetdisk_key";
+ std::vector<MemberScore> mscores(2);
+ std::vector<int>value_size{1,1024};
+ for(int i = 0;i < int(value_size.size()); i++){
+ mscores[i].member = std::string(value_size[i],'a');
+ mscores[i].score = 1.0 * value_size[int(values_.size()) - i - 1];
+ }
+ int ret = 0;
+ uint64_t sum = 0;
+ for (int i = 0; i < int(mscores.size()); i++) {
+ std::vector<MemberScore> tmpv{mscores[i]};
+ sum += 2 * mscores[i].member.size();
+ rocksdb::Status s = zset->Add(key_, 0, &tmpv, &ret);
+ EXPECT_TRUE(s.ok() && ret == 1);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisZSet, &key_size).ok());
+ EXPECT_GE(key_size, sum);
+ }
+ zset->Del(key_);
+}
+
+TEST_F(RedisDiskTest, BitmapDisk) {
+ std::unique_ptr<Redis::Bitmap> bitmap = Util::MakeUnique<Redis::Bitmap>(storage_, "disk_ns_bitmap");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_bitmap");
+ key_ = "bitmapdisk_key";
+ uint32_t offsets[] = {0, 123, 1024*8, 1024*8+1, 3*1024*8, 3*1024*8+1};
+
+ for (int i = 0; i < 6 ;i++) {
+ bool bit = false;
+ bitmap->GetBit(key_, offsets[i], &bit);
+ EXPECT_FALSE(bit);
+ bitmap->SetBit(key_, offsets[i], true, &bit);
+ bitmap->GetBit(key_, offsets[i], &bit);
+ EXPECT_TRUE(bit);
+ // waiting for data write
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ uint64_t key_size;
+ EXPECT_TRUE(disk->GetKeySize(key_, kRedisBitmap, &key_size).ok());
+ EXPECT_GE(key_size, i + 1);
+ }
+ bitmap->Del(key_);
+}
+
+TEST_F(RedisDiskTest, SortedintDisk) {
+ std::unique_ptr<Redis::Sortedint> sortedint = Util::MakeUnique<Redis::Sortedint>(storage_, "disk_ns_sortedint");
+ std::unique_ptr<Redis::Disk> disk = Util::MakeUnique<Redis::Disk>(storage_, "disk_ns_sortedint");
+ key_ = "sortedintdisk_key";
+ int ret;
+ for(int i=0;i<10;i++){
Review Comment:
I noticed that the running results of ci are somewhat different from those of my native machine. At the same time, because this is a draft pr, I am using ci for testing.I found that a byte of data is often not detected due to the problem with the settings of Writebatch(sync is default false). So don't worry. I'll fix it after the test. Thanks again for your serious review
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1250324175
> > > @PragmaTwice @git-hulk @torwig I noticed a problem. In the following tests. When we write a value with only one bit(like 'a'). Occasionally, the size of the key we get is 0. It occurs frequently when the value is very small. I guess the reason for this problem is that sync in writeoption defaults to false. Or because the `GetApproximateSizes` can only yield approximate values, which may be ignored for small values.
> > > ```
> > > std::vector<int> value_size{1, 1024};
> > > for(auto &p : value_size){
> > > EXPECT_TRUE(string->Set(key_, std::string(p, 'a')).ok());
> > > std::string got_value;
> > > EXPECT_TRUE(string->Get(key_, &got_value).ok());
> > > EXPECT_EQ(got_value, std::string(p, 'a'));
> > > std::string value;
> > > string->Get(key_, &value);
> > > uint64_t result = 0;
> > > EXPECT_TRUE(disk->GetKeySize(key_, kRedisString, &result).ok());
> > > EXPECT_GE(result, p);
> > > }
> > > ```
> >
> >
> > @git-hulk Hi. Do you have time to look at this question? Because the approximate usage space is obtained, it may encounter a situation of 0 during testing. Increasing the sleep time also doesn't completely solve the problem. It seems that the sync method is not supported either.
>
> Did you enable the `include_memtabtles` in SizeApproximationOptions?
Of course yes.
```
this->option_.include_memtabtles = true;
this->option_.include_files = true;
```
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1250325471
> > > > > @PragmaTwice @git-hulk @torwig I noticed a problem. In the following tests. When we write a value with only one bit(like 'a'). Occasionally, the size of the key we get is 0. It occurs frequently when the value is very small. I guess the reason for this problem is that sync in writeoption defaults to false. Or because the `GetApproximateSizes` can only yield approximate values, which may be ignored for small values.
> > > > > ```
> > > > > std::vector<int> value_size{1, 1024};
> > > > > for(auto &p : value_size){
> > > > > EXPECT_TRUE(string->Set(key_, std::string(p, 'a')).ok());
> > > > > std::string got_value;
> > > > > EXPECT_TRUE(string->Get(key_, &got_value).ok());
> > > > > EXPECT_EQ(got_value, std::string(p, 'a'));
> > > > > std::string value;
> > > > > string->Get(key_, &value);
> > > > > uint64_t result = 0;
> > > > > EXPECT_TRUE(disk->GetKeySize(key_, kRedisString, &result).ok());
> > > > > EXPECT_GE(result, p);
> > > > > }
> > > > > ```
> > > >
> > > >
> > > > @git-hulk Hi. Do you have time to look at this question? Because the approximate usage space is obtained, it may encounter a situation of 0 during testing. Increasing the sleep time also doesn't completely solve the problem. It seems that the sync method is not supported either.
> > >
> > >
> > > Did you enable the `include_memtabtles` in SizeApproximationOptions?
> >
> >
> > Of course yes.
> > ```
> > this->option_.include_memtabtles = true;
> > this->option_.include_files = true;
> > ```
>
> OK, I need to take some time to inspect the reason. But I think it's ok to skip this corner case first since we should NOT care if the usage is small enough.
It's not only small cases where problems can arise. Sometimes a larger value also has a situation where the result is 0. For example, the failure of this CI has a relatively large value but get key size is zero.
--
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] caipengbo commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
caipengbo commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973937356
##########
src/redis_disk.h:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#pragma once
+
+#include <string>
+#include "redis_db.h"
+#include "redis_metadata.h"
+
+namespace Redis {
+
+class Disk : public Database {
+ public:
+ explicit Disk(Engine::Storage *storage, const std::string &ns): Database(storage, ns) {
+ this->option_.include_memtabtles = true;
+ this->option_.include_files = true;
+ }
+ rocksdb::Status GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft = Slice(),
+ Slice subkeyright = Slice());
+ rocksdb::Status GetStringSize(const Slice &user_key, uint64_t *key_size);
Review Comment:
I think `GetApproximateKeySize` should be a virtual function and every data type should rewrite 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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r975734757
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ return GetStringSize(ns_key, key_size);
+ case RedisType::kRedisHash:
+ return GetHashSize(ns_key, key_size);
+ case RedisType::kRedisBitmap:
+ return GetBitmapSize(ns_key, key_size);
+ case RedisType::kRedisList:
+ return GetListSize(ns_key, key_size);
+ case RedisType::kRedisSet:
+ return GetSetSize(ns_key, key_size);
+ case RedisType::kRedisSortedint:
+ return GetSortedintSize(ns_key, key_size);
+ case RedisType::kRedisZSet:
+ return GetZsetSize(ns_key, key_size);
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
Review Comment:
Why Stream type is not supported? I think getting its size is very similar to `set`, except Stream has its own column family. By the way, you can add a new line before and after `GetKeySize` and delete one line between `GetHashSize` and `GetSetSize`.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r976084369
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ return GetStringSize(ns_key, key_size);
+ case RedisType::kRedisHash:
+ return GetHashSize(ns_key, key_size);
+ case RedisType::kRedisBitmap:
+ return GetBitmapSize(ns_key, key_size);
+ case RedisType::kRedisList:
+ return GetListSize(ns_key, key_size);
+ case RedisType::kRedisSet:
+ return GetSetSize(ns_key, key_size);
+ case RedisType::kRedisSortedint:
+ return GetSortedintSize(ns_key, key_size);
+ case RedisType::kRedisZSet:
+ return GetZsetSize(ns_key, key_size);
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
Review Comment:
Thanks @torwig
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r973585761
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,146 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetStringSize(const Slice &user_key, uint64_t *key_size) {
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ auto key_range = rocksdb::Range(Slice(ns_key), Slice(ns_key + static_cast<char>(0)));
+ return db_->GetApproximateSizes(option_, metadata_cf_handle_, &key_range, 1, key_size);
+}
+
+rocksdb::Status Disk::GetHashSize(const Slice &user_key, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ HashMetadata metadata(false);
+ rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
+ if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
Review Comment:
These code appears in almost every method in this file, so I think you can just define something like
```
rocksdb::Status GetHashSize(auto ns_key, auto metadata, auto key_size) // fill these types
```
and then
```
RedisType type = ...;
std::string ns_key;
AppendNamespacePrefix(user_key, &ns_key);
Metadata metadata(type, false);
rocksdb::Status s = Database::GetMetadata(kRedisHash, ns_key, &metadata);
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
switch (redisType) {
case kRedisHash: GetHashSize(...); break;
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] tanruixiang commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985059376
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <memory>
+#include <string>
+#include <vector>
+#include <gtest/gtest.h>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+#include "redis_stream.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ delete storage_;
Review Comment:
>
Yes. My guess is that there are some resources that must be destructed to be freed, so it can't just be reopened.
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985058768
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <memory>
+#include <string>
+#include <vector>
+#include <gtest/gtest.h>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+#include "redis_stream.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ delete storage_;
Review Comment:
Is this your idea? I didn't delve into it, but it crashes.
```
explicit RedisDiskTest() : TestBase() {
// delete storage_;
storage_->debugconfig();
config_->RocksDB.compression = rocksdb::CompressionType::kNoCompression;
config_->RocksDB.write_buffer_size = 1;
// storage_ = new Engine::Storage(config_);
Status s = storage_->Open();
if (!s.IsOK()) {
std::cout << "Failed to open the storage, encounter error: " << s.Msg() << std::endl;
assert(s.IsOK());
}
storage_->debugconfig();
}
```
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985058039
##########
tests/cppunit/disk_test.cc:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <chrono>
+#include <memory>
+#include <string>
+#include <vector>
+#include <gtest/gtest.h>
+#include "redis_metadata.h"
+#include "test_base.h"
+#include "redis_string.h"
+#include "redis_disk.h"
+#include "redis_set.h"
+#include "redis_list.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_sortedint.h"
+#include "redis_stream.h"
+
+class RedisDiskTest : public TestBase {
+protected:
+ explicit RedisDiskTest() : TestBase() {
+ delete storage_;
Review Comment:
`config_` is a protected member so you can modify its members from the derived class (if I'm not mistaken)
--
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 a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r977602235
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ return GetStringSize(ns_key, key_size);
+ case RedisType::kRedisHash:
+ return GetHashSize(ns_key, key_size);
+ case RedisType::kRedisBitmap:
+ return GetBitmapSize(ns_key, key_size);
+ case RedisType::kRedisList:
+ return GetListSize(ns_key, key_size);
+ case RedisType::kRedisSet:
+ return GetSetSize(ns_key, key_size);
+ case RedisType::kRedisSortedint:
+ return GetSortedintSize(ns_key, key_size);
+ case RedisType::kRedisZSet:
+ return GetZsetSize(ns_key, key_size);
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
Review Comment:
I'm still looking for the reason why sometimes the value obtained is 0. If found, I will add `GetStreamSize`.
--
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] torwig commented on a diff in pull request #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
torwig commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r977598104
##########
src/redis_disk.cc:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+#include <vector>
+#include <memory>
+#include <utility>
+#include <algorithm>
+#include <string>
+
+#include "db_util.h"
+#include "redis_metadata.h"
+#include "redis_sortedint.h"
+#include "rocksdb/status.h"
+#include "redis_zset.h"
+#include "redis_bitmap.h"
+#include "redis_disk.h"
+#include "status.h"
+
+namespace Redis {
+rocksdb::Status Disk::GetApproximateSizes(const Metadata &metadata, const Slice &ns_key,
+ rocksdb::ColumnFamilyHandle *column_family,
+ uint64_t *key_size, Slice subkeyleft,
+ Slice subkeyright) {
+ std::string prefix_key, next_version_prefix_key;
+ InternalKey(ns_key, subkeyleft, metadata.version,
+ storage_->IsSlotIdEncoded()).Encode(&prefix_key);
+ InternalKey(ns_key, subkeyright, metadata.version + 1,
+ storage_->IsSlotIdEncoded()).Encode(&next_version_prefix_key);
+ auto key_range = rocksdb::Range(prefix_key, next_version_prefix_key);
+ uint64_t tmp_size = 0;
+ rocksdb::Status s = db_->GetApproximateSizes(option_, column_family,
+ &key_range, 1, &tmp_size);
+ if (!s.ok()) return s;
+ *key_size += tmp_size;
+ return rocksdb::Status::OK();
+}
+rocksdb::Status Disk::GetKeySize(const Slice &user_key, RedisType type, uint64_t *key_size) {
+ *key_size = 0;
+ std::string ns_key;
+ AppendNamespacePrefix(user_key, &ns_key);
+ switch (type) {
+ case RedisType::kRedisString:
+ return GetStringSize(ns_key, key_size);
+ case RedisType::kRedisHash:
+ return GetHashSize(ns_key, key_size);
+ case RedisType::kRedisBitmap:
+ return GetBitmapSize(ns_key, key_size);
+ case RedisType::kRedisList:
+ return GetListSize(ns_key, key_size);
+ case RedisType::kRedisSet:
+ return GetSetSize(ns_key, key_size);
+ case RedisType::kRedisSortedint:
+ return GetSortedintSize(ns_key, key_size);
+ case RedisType::kRedisZSet:
+ return GetZsetSize(ns_key, key_size);
+ case RedisType::kRedisNone:
+ return rocksdb::Status::NotFound("Not found ", user_key);
+ case RedisType::kRedisStream:
+ return rocksdb::Status::NotSupported("Not support stream");
Review Comment:
@tanruixiang I've added Stream design here https://github.com/apache/incubator-kvrocks-website/pull/17 .
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r990078770
##########
tests/gocase/unit/disk/disk_test.go:
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package disk
+
+import (
+ "context"
+ "strconv"
+ "strings"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func TestDisk(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{
+ "rocksdb.compression": "no",
+ "rocksdb.write_buffer_size": "1",
+ "rocksdb.block_size": "100",
+ })
+ defer srv.Close()
+ ctx := context.Background()
+ rdb := srv.NewClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+ estimationFactor := 0.1
+ t.Run("Disk usage String", func(t *testing.T) {
+ require.NoError(t, rdb.Set(ctx, "stringkey", strings.Repeat("A", 1024), 0).Err())
+ val, err := rdb.Do(ctx, "Disk", "usage", "stringkey").Int()
+ require.NoError(t, err)
+ require.GreaterOrEqual(t, val, int(estimationFactor*1024))
+ require.LessOrEqual(t, val, int(1024/estimationFactor))
+ })
+
+ t.Run("Disk usage Hash", func(t *testing.T) {
+ approximateSize := 0
+ for i := 0; i < 10000; i++ {
+ require.NoError(t, rdb.HSet(ctx, "hashkey", "key"+strconv.Itoa(i), "value"+strconv.Itoa(i)).Err())
+ approximateSize += len("hashkey") + len("key"+strconv.Itoa(i)) + len("value"+strconv.Itoa(i)) + 8
+ }
+ require.NoError(t, rdb.Set(ctx, "hashkey", strings.Repeat("A", 1024), 0).Err())
+ val, err := rdb.Do(ctx, "Disk", "usage", "hashkey").Int()
+ require.NoError(t, err)
+ require.GreaterOrEqual(t, val, int(float64(approximateSize)*estimationFactor))
+ require.LessOrEqual(t, val, int(float64(approximateSize)/estimationFactor))
Review Comment:
I think it is fine if rocksdb also has a large range.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r990209248
##########
src/redis_cmd.cc:
##########
@@ -3476,6 +3477,32 @@ class CommandInfo : public Commander {
}
};
+class CommandDisk : public Commander {
+ public:
+ Status Parse(const std::vector<std::string> &args) override {
+ std::string opname = Util::ToLower(args[1]);
+ if (opname != "usage")
+ return Status(Status::RedisInvalidCmd, "Unknown operation");
+ if (args.size() != 3)
+ return Status(Status::RedisInvalidCmd, "Incorrect number of parameters");
Review Comment:
Can remove this since it's done before executing the command
```suggestion
```
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#discussion_r985279455
##########
tests/gocase/unit/disk/disk_test.go:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package disk
+
+import (
+ "context"
+ "strconv"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func TestDisk(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{
+ "rocksdb.compression": "no",
+ "rocksdb.write_buffer_size": "1",
+ })
+ defer srv.Close()
+ ctx := context.Background()
+ rdb := srv.NewClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+ estimationFactor := 0.5
+ t.Run("Disk usage String", func(t *testing.T) {
+ require.NoError(t, rdb.Set(ctx, "stringkey", "aaaaaaaaaaaaaaaaaaaaaaaa", 0).Err())
+ val, err := rdb.Do(ctx, "Disk", "usage", "stringkey").Int()
+ require.NoError(t, err)
+ require.GreaterOrEqual(t, val, 1)
Review Comment:
IMHO, the expected value is weird. And I wonder if it can be a range rather than only a minimum value.
##########
tests/gocase/unit/disk/disk_test.go:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package disk
+
+import (
+ "context"
+ "strconv"
+ "testing"
+
+ "github.com/apache/incubator-kvrocks/tests/gocase/util"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+)
+
+func TestDisk(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{
+ "rocksdb.compression": "no",
+ "rocksdb.write_buffer_size": "1",
+ })
+ defer srv.Close()
+ ctx := context.Background()
+ rdb := srv.NewClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+ estimationFactor := 0.5
+ t.Run("Disk usage String", func(t *testing.T) {
+ require.NoError(t, rdb.Set(ctx, "stringkey", "aaaaaaaaaaaaaaaaaaaaaaaa", 0).Err())
+ val, err := rdb.Do(ctx, "Disk", "usage", "stringkey").Int()
+ require.NoError(t, err)
+ require.GreaterOrEqual(t, val, 1)
Review Comment:
IMHO, the expected value is weird. And I wonder if it can be a range rather than only a lower bound.
--
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 #882: Support DISK Command
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #882:
URL: https://github.com/apache/incubator-kvrocks/pull/882#issuecomment-1271569851
@git-hulk @torwig @caipengbo Hi. Do you have time to review?
--
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