Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/16135 )
Change subject: Upgrade C++ to protobuf 3.12 ...................................................................... Patch Set 4: (13 comments) http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/cfile/bloomfile.cc File src/kudu/cfile/bloomfile.cc: http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/cfile/bloomfile.cc@173 PS4, Line 173: hdr.ByteSizeLong() > Maybe, wrap into static_cast<uint32_t>() to be explicit on what's expected Done http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/consensus/consensus_queue.cc@696 PS4, Line 696: LAGS_consensus_max_batch_size_bytes - request->ByteSizeLong(); > While you are here, do you mind adding a few lines to verify that FLAGS_con I'm afraid that's a behavioral change and don't want to let this "upgrade PB" patch creep into semantic differences http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/consensus/log.cc@1240 PS4, Line 1240: total_size_bytes_( : PREDICT_FALSE(count == 1 && entry_batch_pb_.entry(0).type() == FLUSH_MARKER) : ? 0 : entry_batch_pb_.ByteSizeLong()) > Does it make sense to change type of LogEntryBatch::total_size_bytes_ to si Done http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/consensus/log_cache.h File src/kudu/consensus/log_cache.h: http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/consensus/log_cache.h@76 PS4, Line 76: ByteSize > ByteSizeLong() Done http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/consensus/log_util.cc File src/kudu/consensus/log_util.cc: http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/consensus/log_util.cc@785 PS4, Line 785: new_header.ByteSizeLong() > nit: wrap into static_cast<uint32_t>() ? Done http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/consensus/log_util.cc@807 PS4, Line 807: footer.ByteSizeLong() > ditto Done http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/rpc/inbound_call.cc File src/kudu/rpc/inbound_call.cc: http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/rpc/inbound_call.cc@181 PS4, Line 181: response.ByteSizeLong(); > What if response.ByteSizeLong() > 2^32 ? Does it make sense to add CHECK() done http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/rpc/outbound_call.cc File src/kudu/rpc/outbound_call.cc: http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/rpc/outbound_call.cc@148 PS4, Line 148: req.ByteSizeLong(); > Add DCHECK() on ByteSizeLong() to be less that 2^32 ? Done http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/rpc/result_tracker.h File src/kudu/rpc/result_tracker.h: http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/rpc/result_tracker.h@283 PS4, Line 283: int64_t > Does it make sense to switch this to size_t along with kudu_malloc_usable_s perhaps but I didn't want to make more changes than necessary here lest some unintended behavioral change leak in http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/tools/tool_action_pbc.cc File src/kudu/tools/tool_action_pbc.cc: http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/tools/tool_action_pbc.cc@205 PS4, Line 205: JsonParseOptions opts; : opts.case_insensitive_enum_parsing = true; > nit: move this outside of the cycle? Done http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/util/pb_util.cc File src/kudu/util/pb_util.cc: http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/util/pb_util.cc@793 PS4, Line 793: DCHECK_GE(data_len_long, 0) << "Error computing ByteSize"; > Is this effective anymore? Done http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/util/pb_util.cc@808 PS4, Line 808: static_cast<uint32_t> > drop? Done http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/util/protobuf_util.h File src/kudu/util/protobuf_util.h: http://gerrit.cloudera.org:8080/#/c/16135/4/src/kudu/util/protobuf_util.h@37 PS4, Line 37: ByteSize > ByteSizeLong Done -- To view, visit http://gerrit.cloudera.org:8080/16135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifd5e6bbdd2c517b1af2ff833701983315111bba4 Gerrit-Change-Number: 16135 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 07 Jul 2020 22:04:07 +0000 Gerrit-HasComments: Yes
