Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8770 )
Change subject: IMPALA-6262: Always initialize runtime profile for DataSink ...................................................................... Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/8770/2/be/src/exec/hdfs-table-sink.cc File be/src/exec/hdfs-table-sink.cc: http://gerrit.cloudera.org:8080/#/c/8770/2/be/src/exec/hdfs-table-sink.cc@61 PS2, Line 61: GetName() Are you sure it's safe to call a virtual function during the constructor? It's also a bit dangerous to call this because you don't really know that GetName() doesn't depend on some other object state (luckily, it looks like all GetName implementations return a static string). Given that we're calling GetName() from the derived classes now, let's make GetName() not virtual to avoid the first problem. For the second problem, you may want to just use the static string at these places and get rid of GetName(), but I feel less strongly about that. -- To view, visit http://gerrit.cloudera.org:8080/8770 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a683000ef180027b929dbebe78bc2a530a4767e Gerrit-Change-Number: 8770 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Kim Jin Chul <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 06 Dec 2017 16:58:40 +0000 Gerrit-HasComments: Yes
