Dan Burkert has posted comments on this change. Change subject: KUDU-2124. Don't hold session lock while initializing a TabletCopySession ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/7985/1/src/kudu/tserver/tablet_copy_service.cc File src/kudu/tserver/tablet_copy_service.cc: Line 171: RPC_RETURN_NOT_OK(session->Init(), > It would be nice to remember if we were the thread to insert the session in Done Line 249: RPC_RETURN_NOT_OK(session->Init(), > Instead of waiting, we could simply reject the request if (!initted()) Done http://gerrit.cloudera.org:8080/#/c/7985/1/src/kudu/tserver/tablet_copy_source_session.cc File src/kudu/tserver/tablet_copy_source_session.cc: Line 166: shared_ptr<log::LogReader> reader = tablet_replica_->log()->reader(); > This is an existing bug, but we should take a ref to tablet_replica_->log() Hmm, I'm not seeing where the log_ field in TabletReplica is getting reset? The header docs for log() also don't mention this. Line 389: Status s; > no longer used Done http://gerrit.cloudera.org:8080/#/c/7985/1/src/kudu/tserver/tablet_copy_source_session.h File src/kudu/tserver/tablet_copy_source_session.h: PS1, Line 33: "kudu/gutil/integral_types.h" > Huh. I didn't know about this file. I've always used <stdint.h> not sure, this is just a reorder. PS1, Line 192: scoped_refptr<tablet::TabletReplica> > nit: This can be made const (I know you haven't worked on this code before Done Line 197: KuduOnceDynamic init_once_; > Would be nice to add a comment noting that once the object is initialized, Added something to that effect. -- To view, visit http://gerrit.cloudera.org:8080/7985 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If8bd295a59ade8c89fdf1853dd64c4bceca8da91 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-HasComments: Yes
