Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10843 )
Change subject: IMPALA-4784: Remove InProcessStatestore ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/10843/3/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/10843/3/be/src/exprs/expr-test.cc@8738 PS3, Line 8738: Statestore* statestore = new Statestore(metrics.get()); > Would it make sense to make this a global scoped_ptr or similar? Seems triv I made this global, but making it a scoped pointer has problems due to no clean shutdown semantics for the statestore. See IMPALA-7235. http://gerrit.cloudera.org:8080/#/c/10843/3/be/src/service/session-expiry-test.cc File be/src/service/session-expiry-test.cc: http://gerrit.cloudera.org:8080/#/c/10843/3/be/src/service/session-expiry-test.cc@53 PS3, Line 53: Statestore* statestore = new Statestore(metrics.get()); > Same question Same as other comments: IMPALA-7235 http://gerrit.cloudera.org:8080/#/c/10843/3/be/src/statestore/statestore-test.cc File be/src/statestore/statestore-test.cc: http://gerrit.cloudera.org:8080/#/c/10843/3/be/src/statestore/statestore-test.cc@40 PS3, Line 40: Statestore* statestore = new Statestore(metrics.get()); > Same question here and below. Could also use the IGNORE_LEAKING_OBJECT macr Unfortunately, the Statestore does not have clean shut down semantics since it's expected that it will run for the life of the cluster. Due to that having a scoped_ptr for the Statestore will cause crashes since some of the threads it spawns run functions that have no exit conditions. I think this is why we leak the InProcessStatestore object in pretty much every BE test we use it in. It's possible to have the Statestore shut down cleanly, but I'd rather do that as a separate patch, since it could have edge cases of its own. For now, I'll use IGNORE_LEAKING_OBJECT in the tests. -- To view, visit http://gerrit.cloudera.org:8080/10843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2621873e593b36c9612a6402ac6c5d8e3b49cde9 Gerrit-Change-Number: 10843 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 02 Jul 2018 21:21:10 +0000 Gerrit-HasComments: Yes
