[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10843 ) Change subject: IMPALA-4784: Remove InProcessStatestore .. IMPALA-4784: Remove InProcessStatestore InProcessStatestore was only used by statestore-test, expr-test and session-expiry-test. With a slight refactor of the Statestore class, InProcessStatestore becomes obsolete. This patch moves the ownership of the ThriftServer into the Statestore class and Statestore::Init() now takes a 'port' parameter instead of using the FLAGS_state_store_port directly. We also remove the InProcessStatestore completely. A follow on patch will get rid of the InProcessImpalaServer too (IMPALA-6013) Testing: Ran 'core' tests. Change-Id: I2621873e593b36c9612a6402ac6c5d8e3b49cde9 Reviewed-on: http://gerrit.cloudera.org:8080/10843 Reviewed-by: Sailesh Mukil Tested-by: Impala Public Jenkins --- M be/src/exprs/expr-test.cc M be/src/service/session-expiry-test.cc M be/src/statestore/statestore-test.cc M be/src/statestore/statestore.cc M be/src/statestore/statestore.h M be/src/statestore/statestored-main.cc M be/src/testutil/in-process-servers.cc M be/src/testutil/in-process-servers.h 8 files changed, 95 insertions(+), 125 deletions(-) Approvals: Sailesh Mukil: Looks good to me, approved Impala Public Jenkins: Verified -- 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: merged Gerrit-Change-Id: I2621873e593b36c9612a6402ac6c5d8e3b49cde9 Gerrit-Change-Number: 10843 Gerrit-PatchSet: 7 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10843 ) Change subject: IMPALA-4784: Remove InProcessStatestore .. Patch Set 6: Verified+1 -- 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: 6 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 03 Jul 2018 08:21:47 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10843 ) Change subject: IMPALA-4784: Remove InProcessStatestore .. Patch Set 6: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2769/ DRY_RUN=false -- 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: 6 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 03 Jul 2018 05:00:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Hello Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10843 to look at the new patch set (#6). Change subject: IMPALA-4784: Remove InProcessStatestore .. IMPALA-4784: Remove InProcessStatestore InProcessStatestore was only used by statestore-test, expr-test and session-expiry-test. With a slight refactor of the Statestore class, InProcessStatestore becomes obsolete. This patch moves the ownership of the ThriftServer into the Statestore class and Statestore::Init() now takes a 'port' parameter instead of using the FLAGS_state_store_port directly. We also remove the InProcessStatestore completely. A follow on patch will get rid of the InProcessImpalaServer too (IMPALA-6013) Testing: Ran 'core' tests. Change-Id: I2621873e593b36c9612a6402ac6c5d8e3b49cde9 --- M be/src/exprs/expr-test.cc M be/src/service/session-expiry-test.cc M be/src/statestore/statestore-test.cc M be/src/statestore/statestore.cc M be/src/statestore/statestore.h M be/src/statestore/statestored-main.cc M be/src/testutil/in-process-servers.cc M be/src/testutil/in-process-servers.h 8 files changed, 95 insertions(+), 125 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/10843/6 -- 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: newpatchset Gerrit-Change-Id: I2621873e593b36c9612a6402ac6c5d8e3b49cde9 Gerrit-Change-Number: 10843 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10843 ) Change subject: IMPALA-4784: Remove InProcessStatestore .. Patch Set 6: Code-Review+2 Hit a clang-tidy issue. Rebase, carry +2. -- 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: 6 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 03 Jul 2018 04:59:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10843 ) Change subject: IMPALA-4784: Remove InProcessStatestore .. Patch Set 5: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/2766/ -- 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: 5 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 03 Jul 2018 01:14:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10843 ) Change subject: IMPALA-4784: Remove InProcessStatestore .. Patch Set 5: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2766/ DRY_RUN=false -- 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: 5 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 02 Jul 2018 21:48:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10843 ) Change subject: IMPALA-4784: Remove InProcessStatestore .. Patch Set 5: Code-Review+2 -- 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: 5 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 02 Jul 2018 21:48:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10843 ) Change subject: IMPALA-4784: Remove InProcessStatestore .. Patch Set 4: Code-Review+2 -- 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: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 02 Jul 2018 21:44:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
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 Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 02 Jul 2018 21:21:10 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10843 to look at the new patch set (#4). Change subject: IMPALA-4784: Remove InProcessStatestore .. IMPALA-4784: Remove InProcessStatestore InProcessStatestore was only used by statestore-test, expr-test and session-expiry-test. With a slight refactor of the Statestore class, InProcessStatestore becomes obsolete. This patch moves the ownership of the ThriftServer into the Statestore class and Statestore::Init() now takes a 'port' parameter instead of using the FLAGS_state_store_port directly. We also remove the InProcessStatestore completely. A follow on patch will get rid of the InProcessImpalaServer too (IMPALA-6013) Testing: Ran 'core' tests. Change-Id: I2621873e593b36c9612a6402ac6c5d8e3b49cde9 --- M be/src/exprs/expr-test.cc M be/src/service/session-expiry-test.cc M be/src/statestore/statestore-test.cc M be/src/statestore/statestore.cc M be/src/statestore/statestore.h M be/src/statestore/statestored-main.cc M be/src/testutil/in-process-servers.cc M be/src/testutil/in-process-servers.h 8 files changed, 95 insertions(+), 125 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/10843/4 -- 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: newpatchset Gerrit-Change-Id: I2621873e593b36c9612a6402ac6c5d8e3b49cde9 Gerrit-Change-Number: 10843 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10843 ) Change subject: IMPALA-4784: Remove InProcessStatestore .. Patch Set 3: (3 comments) Seems good, thanks for the cleanup! 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 trivial to avoid the memory leak now. 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 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 macro. I figure it's easier to fix this now than have someone else with less context try to fix it later when enabling leak santizier. -- 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 Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 02 Jul 2018 19:05:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10843 to look at the new patch set (#3). Change subject: IMPALA-4784: Remove InProcessStatestore .. IMPALA-4784: Remove InProcessStatestore InProcessStatestore was only used by statestore-test, expr-test and session-expiry-test. With a slight refactor of the Statestore class, InProcessStatestore becomes obsolete. This patch moves the ownership of the ThriftServer into the Statestore class and Statestore::Init() now takes a 'port' parameter instead of using the FLAGS_state_store_port directly. We also remove the InProcessStatestore completely. A follow on patch will get rid of the InProcessImpalaServer too (IMPALA-6013) Testing: Ran 'core' tests. Change-Id: I2621873e593b36c9612a6402ac6c5d8e3b49cde9 --- M be/src/exprs/expr-test.cc M be/src/service/session-expiry-test.cc M be/src/statestore/statestore-test.cc M be/src/statestore/statestore.cc M be/src/statestore/statestore.h M be/src/statestore/statestored-main.cc M be/src/testutil/in-process-servers.cc M be/src/testutil/in-process-servers.h 8 files changed, 74 insertions(+), 119 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/10843/3 -- 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: newpatchset Gerrit-Change-Id: I2621873e593b36c9612a6402ac6c5d8e3b49cde9 Gerrit-Change-Number: 10843 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10843 ) Change subject: IMPALA-4784: Remove InProcessStatestore .. Patch Set 2: Code-Review-1 Found a bug. Will upload a new patchset shortly. -- 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: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 29 Jun 2018 21:34:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10843 to look at the new patch set (#2). Change subject: IMPALA-4784: Remove InProcessStatestore .. IMPALA-4784: Remove InProcessStatestore InProcessStatestore was only used by statestore-test and with a slight refactor of the Statestore class, InProcessStatestore becomes obsolete. This patch moves the ownership of the ThriftServer into the Statestore class and Statestore::Init() now takes a 'port' parameter instead of using the FLAGS_state_store_port directly. We also remove the InProcessStatestore completely. A follow on patch will get rid of the InProcessImpalaServer too (IMPALA-6013) Testing: Ran 'core' tests. Change-Id: I2621873e593b36c9612a6402ac6c5d8e3b49cde9 --- M be/src/statestore/statestore-test.cc M be/src/statestore/statestore.cc M be/src/statestore/statestore.h M be/src/statestore/statestored-main.cc M be/src/testutil/in-process-servers.cc M be/src/testutil/in-process-servers.h 6 files changed, 62 insertions(+), 113 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/10843/2 -- 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: newpatchset Gerrit-Change-Id: I2621873e593b36c9612a6402ac6c5d8e3b49cde9 Gerrit-Change-Number: 10843 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4784: Remove InProcessStatestore
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10843 Change subject: IMPALA-4784: Remove InProcessStatestore .. IMPALA-4784: Remove InProcessStatestore InProcessStatestore was only used by statestore-test and with a slight refactor of the Statestore class, InProcessStatestore becomes obsolete. This patch moves the ownership of the ThriftServer into the Statestore class and Statestore::Init() now takes a 'port' parameter instead of using the FLAGS_state_store_port directly. We also remove the InProcessStatestore completely. A follow on patch will get rid of the InProcessImpalaServer too (IMPALA-6013) Testing: Ran 'core' tests. Change-Id: I2621873e593b36c9612a6402ac6c5d8e3b49cde9 --- M be/src/statestore/statestore-test.cc M be/src/statestore/statestore.cc M be/src/statestore/statestore.h M be/src/statestore/statestored-main.cc M be/src/testutil/in-process-servers.cc M be/src/testutil/in-process-servers.h 6 files changed, 63 insertions(+), 110 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/43/10843/1 -- 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: newchange Gerrit-Change-Id: I2621873e593b36c9612a6402ac6c5d8e3b49cde9 Gerrit-Change-Number: 10843 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil
