Dan Burkert has posted comments on this change.

Change subject: KUDU-1358 (part 2): heartbeat to every master
......................................................................


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3610/6/src/kudu/integration-tests/ts_tablet_manager-itest.cc
File src/kudu/integration-tests/ts_tablet_manager-itest.cc:

Line 176:         if (reports[0].updated_tablets_size()) break;
!reports[0].updated_tablets().empty()


http://gerrit.cloudera.org:8080/#/c/3610/6/src/kudu/tserver/heartbeater.cc
File src/kudu/tserver/heartbeater.cc:

Line 206:     for (int i = first_failure_idx - 1; i >= 0; i--) {
I think this would be more straightforward as a forward loop:

for (int i = 0; i < first_failure_idx; i++)

you might also simplify some of the locals by putting it inside the failure 
claus in the first loop.


Line 384:                         "Failed to send heartbeat to master");
should this have the master addr as well?


http://gerrit.cloudera.org:8080/#/c/3610/6/src/kudu/tserver/ts_tablet_manager.h
File src/kudu/tserver/ts_tablet_manager.h:

Line 153:   void PopulateTabletReport(master::TabletReportPB* report,
this API is pretty funky, any reason not to have separate 
PopulateFullTabletReport and PopulatePartialTabletReport(TabletReportPB*, const 
vector<string>&) methods?  Doesn't look like the implementation reuses much.


-- 
To view, visit http://gerrit.cloudera.org:8080/3610
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic85ac4193462d21c989dbd7874b451e8eaab8e3e
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to