acelyc111 commented on code in PR #1091:
URL: https://github.com/apache/incubator-pegasus/pull/1091#discussion_r942236139
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2274,7 +2294,8 @@
pegasus_server_impl::append_key_value_for_scan(std::vector<::dsn::apps::key_valu
uint32_t epoch_now,
bool no_value,
bool request_validate_hash,
- bool request_expire_ts)
+ bool request_expire_ts,
+ bool fill_value)
Review Comment:
The function name `append_key_value_for_scan` is not suitable now, because
the 'append' operation will not happen when fill_value is false.
You can do a refactor to separete then key-value valicate part and append
part into two functions.
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -1198,7 +1198,8 @@ void pegasus_server_impl::on_get_scanner(get_scanner_rpc
rpc)
epoch_now,
request.no_value,
request.__isset.validate_partition_hash ?
request.validate_partition_hash : true,
- return_expire_ts);
+ return_expire_ts,
+ !request.only_return_count);
Review Comment:
I found you reverse the value every calls of append_key_value_for_scan, why
not use it directly?
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -2316,24 +2332,28 @@
pegasus_server_impl::append_key_value_for_scan(std::vector<::dsn::apps::key_valu
return range_iteration_state::kFiltered;
}
}
- std::shared_ptr<char>
key_buf(::dsn::utils::make_shared_array<char>(raw_key.length()));
- ::memcpy(key_buf.get(), raw_key.data(), raw_key.length());
- kv.key.assign(std::move(key_buf), 0, raw_key.length());
+ if (fill_value) {
Review Comment:
Sorry, I didn't get your point..
I think short-circut return is often a recommand code style too.
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -1389,6 +1401,11 @@ void pegasus_server_impl::on_scan(scan_rpc rpc)
it->Next();
}
+ if (!only_return_count) {
+ resp.kvs.emplace_back(::dsn::apps::key_value());
Review Comment:
Same
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -1389,6 +1401,11 @@ void pegasus_server_impl::on_scan(scan_rpc rpc)
it->Next();
}
+ if (!only_return_count) {
Review Comment:
should be `if (only_return_count)` ?
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -1221,6 +1222,10 @@ void pegasus_server_impl::on_get_scanner(get_scanner_rpc
rpc)
it->Next();
}
+ if (request.only_return_count) {
+ resp.kvs.emplace_back(::dsn::apps::key_value());
Review Comment:
Why fill such a empty kv-pair, is it neccessary?
##########
src/server/pegasus_server_impl.cpp:
##########
@@ -1297,7 +1303,10 @@ void pegasus_server_impl::on_get_scanner(get_scanner_rpc
rpc)
_pfc_recent_filter_count->add(filter_count);
}
- _cu_calculator->add_scan_cu(req, resp.error, resp.kvs);
+ // abandon calculate capacity unit
+ if (!request.only_return_count) {
Review Comment:
We should also calculate CU, and you have to update add_scan_cu, add some
small CU at least.
##########
src/client_lib/pegasus_scanner_impl.cpp:
##########
@@ -162,13 +168,16 @@ void
pegasus_client_impl::pegasus_scanner_impl::_async_next_internal()
}
// valid data got
- std::string hash_key, sort_key;
- pegasus_restore_key(_kvs[_p].key, hash_key, sort_key);
- std::string value(_kvs[_p].value.data(), _kvs[_p].value.length());
- uint32_t expire_ts_seconds = _kvs[_p].__isset.expire_ts_seconds
- ?
static_cast<uint32_t>(_kvs[_p].expire_ts_seconds)
- : 0;
-
+ std::string hash_key, sort_key, value;
+ uint32_t expire_ts_seconds = 0;
+ // _kv_count == -1 means req just want to get data counts, not include
data value
+ if (_kv_count == -1) {
Review Comment:
should be _kv_count != -1 ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
