Skip to content

Conversation

@marete
Copy link
Collaborator

@marete marete commented Jul 17, 2025

This change is Reviewable

marete and others added 2 commits July 15, 2025 22:46
…gth are adjacent.

This will be used in a subsequent PR to correctly determine if 2 Bigtable row
keys are adjacent, given that a Bigtable row key must be at most 4KiB
in length.
@marete marete requested review from dopiera and prawilny July 17, 2025 18:00
@marete marete force-pushed the limit_row_key_length branch from 2acfcf9 to bde2e54 Compare July 17, 2025 18:45
@marete marete force-pushed the limit_row_key_length branch from c3c57be to 1001bda Compare July 17, 2025 20:14
Copy link
Collaborator

@prawilny prawilny left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dopiera)


google/cloud/bigtable/emulator/table.cc line 796 at r1 (raw file):

  auto const& row_prefix = request.row_key_prefix();
  if (request.row_key_prefix().size() > kMaxRowLen) {

nit: There are two ifs regarding request.row_key_preifx(), which is refered to by both request.row_key_prefix and row_prefix.
Please make both conditions use the same name so that it's clear that they both validate the same thing.


google/cloud/bigtable/emulator/table.cc line 803 at r1 (raw file):

            absl::StrFormat("%zu", request.row_key_prefix().size())));
  }

nit: extra line, which clouds the fact that both conditions validate the same value. Maybe let's remove it?


google/cloud/bigtable/emulator/limits.h line 23 at r1 (raw file):

namespace bigtable {
namespace emulator {
constexpr std::size_t kMaxRowLen = 2 << 21;

Isn't our constant (2**21) like 2MB, which is significantly higher than 4KiB we claim elsewhere?

Doesn't it suggest that we should store the string for human readable limit next to the value so that it can be easier to verify that they match?


google/cloud/bigtable/google_cloud_cpp_bigtable.bzl line 207 at r1 (raw file):

    "internal/rate_limiter.cc",
    "internal/readrowsparser.cc",
    "internal/retry_context.cc",

Was it intended?

@marete marete requested a review from prawilny July 28, 2025 19:07
Copy link
Collaborator Author

@marete marete left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @dopiera and @prawilny)


google/cloud/bigtable/emulator/limits.h line 23 at r1 (raw file):

Previously, prawilny (Adam Czajkowski) wrote…

Isn't our constant (2**21) like 2MB, which is significantly higher than 4KiB we claim elsewhere?

Doesn't it suggest that we should store the string for human readable limit next to the value so that it can be easier to verify that they match?

Thank you very much for this excellent catch. Meant 2 << 11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants