Todd Lipcon has posted comments on this change. Change subject: WIP: Expose "raw" mode in KuduScanner and allow to pass flags ......................................................................
Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/6624/5/src/kudu/client/client.h File src/kudu/client/client.h: Line 2018: enum { kDefaultRawModeFlags = -1 }; > This is confusing. If the idea is to provide a constant that logically mean yea, I'd expect the default to be 0 http://gerrit.cloudera.org:8080/#/c/6624/5/src/kudu/client/scan_configuration.cc File src/kudu/client/scan_configuration.cc: Line 182: raw_mode_flags_ = flags; we should probably check that there are no unsupported flags set http://gerrit.cloudera.org:8080/#/c/6624/5/src/kudu/client/scan_configuration.h File src/kudu/client/scan_configuration.h: Line 139: return raw_mode_flags_ != KuduScanner::kDefaultRawModeFlags; strikes me as odd, since this is -1 (all flags set) http://gerrit.cloudera.org:8080/#/c/6624/5/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: PS5, Line 536: raw_mode_flags_ != KuduScanner::kDefaultRawModeFlags yea, this defaultRawModeFlags thing is funny looking here - I'd expect the default "0" to mean not enabled, and maybe allocate a flag called 'RAW_MODE_V1" or something? That way if we ever changed our internal format, we could switch to setting RAW_MODE_V2 instead so the client can indicate its compatibility with the current version Line 550: LOG(FATAL) << "Cannot extract rows, \"raw\" mode"; would it be possible for a user in a release build to hit this case? Or would this really be indicative of a bug on our side? http://gerrit.cloudera.org:8080/#/c/6624/5/src/kudu/tserver/scanners.h File src/kudu/tserver/scanners.h: Line 100: int64_t raw_mode_flags() const { return raw_mode_flags_; } > Why is this a ScannerManager thing? I thought it'd only be per scanner? yea http://gerrit.cloudera.org:8080/#/c/6624/5/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: Line 410: void set_raw_mode_flags(int64_t raw_mode_flags) override { > This is unintuitive; since collectors are constructed per RPC, I would have would it be simpler if we pass the raw-mode flags on every ContinueScan RPC rather than storing it as part of the scanner object? Then we would be able to avoid the changes to the Scanner interface, as well as solve this problem http://gerrit.cloudera.org:8080/#/c/6624/5/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: Line 270: // "Raw" mode flags. > I understand the desire for a "raw" mode client-side, so that users who sig +1, it shouldn't distinguish raw vs not-raw. At this level all scans are kind of "raw" Perhaps it should be called "format_flags" or something? As for a flag bitset vs separate booleans I dont have a strong opinion. I suppose separate booleans would be less dense in memory than a bitset, but doubt it matters. -- To view, visit http://gerrit.cloudera.org:8080/6624 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
