Copilot commented on code in PR #3504:
URL: https://github.com/apache/kvrocks/pull/3504#discussion_r3442688545
##########
src/commands/cmd_stream.cc:
##########
@@ -49,6 +50,48 @@ CommandKeyRange ParseStreamReadRange(const
std::vector<std::string> &args, uint3
range.last_key = range.first_key + stream_size - 1;
return range;
}
+
+// Redis accepts only canonical positive decimal numids here;
+// reject forms like +1 or 01 before integer parsing.
+bool IsXAckDelNumIDs(std::string_view input) {
+ if (input.empty() || input[0] < '1' || input[0] > '9') return false;
+
+ return std::all_of(input.begin() + 1, input.end(), [](char c) { return c >=
'0' && c <= '9'; });
+}
+
+StatusOr<uint64_t> ParseXAckDelStreamEntryIDComponent(std::string_view input,
bool allow_negative_zero) {
+ if (input.empty()) return {Status::RedisParseErr,
redis::kErrInvalidEntryIdSpecified};
+
+ if (input[0] == '+') {
+ input.remove_prefix(1);
+ } else if (input[0] == '-') {
+ if (!allow_negative_zero) return {Status::RedisParseErr,
redis::kErrInvalidEntryIdSpecified};
+ input.remove_prefix(1);
+ if (input.empty() || !std::all_of(input.begin(), input.end(), [](char c) {
return c == '0'; })) {
+ return {Status::RedisParseErr, redis::kErrInvalidEntryIdSpecified};
+ }
+ return 0;
+ }
+
+ auto parsed = ParseInt<uint64_t>(input, 10);
+ if (!parsed) return {Status::RedisParseErr,
redis::kErrInvalidEntryIdSpecified};
+ return *parsed;
+}
+
+Status ParseXAckDelStreamEntryID(const std::string &input,
redis::StreamEntryID *id) {
+ auto pos = input.find('-');
+ if (pos != std::string::npos) {
+ auto ms =
GET_OR_RET(ParseXAckDelStreamEntryIDComponent(std::string_view(input).substr(0,
pos), false));
+ auto seq =
GET_OR_RET(ParseXAckDelStreamEntryIDComponent(std::string_view(input).substr(pos
+ 1), true));
+ id->ms = ms;
+ id->seq = seq;
+ } else {
+ auto ms = GET_OR_RET(ParseXAckDelStreamEntryIDComponent(input, false));
+ id->ms = ms;
+ id->seq = 0;
+ }
+ return Status::OK();
Review Comment:
`ParseXAckDelStreamEntryID()` allows `ms == UINT64_MAX`, but the stream ID
encoding uses `UINT64_MAX` as an internal delimiter (see
`StreamEntryID::Maximum()` using `UINT64_MAX - 1` for `ms`). Accepting `ms ==
UINT64_MAX` can create keys that are indistinguishable from internal stream
subkeys and break stream operations/replication. It should reject IDs with `ms
> StreamEntryID::Maximum().ms`.
##########
src/cluster/batch_sender.cc:
##########
@@ -58,6 +60,7 @@ Status BatchSender::PutLogData(const rocksdb::Slice &blob) {
if (!s.ok()) {
return {Status::NotOK, fmt::format("failed to put log data to migration
batch, {}", s.ToString())};
}
+ pending_logdata_only_ = true;
Review Comment:
`pending_logdata_only_` is set to `true` for every `PutLogData()` call, even
when the batch already contains Put/Delete records. That makes `IsFull()`
return false and can allow mixed (data+logdata) batches to exceed `max_bytes_`.
Consider keeping this flag true only while the batch is still logdata-only
(i.e., no data records have been added 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
