You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2019/10/02 05:57:46 UTC
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14345
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
[mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
This patch makes it possible to start ExternalMiniCluster on a directory
structure created by its prior instance if using the masters' RPC
end-points from the very first run.
This functionality is useful for test scenarios like
MasterMigrationTest.TestEndToEndMigration, when a Kudu master catalog
is preserved between cluster's restarts.
The motivation for this patch was seeing the mentioned test scenario
failed if running with the built-in NTP: mini_chronyd wasn't recreating
the configuration file for chronyd (the test NTP server) as necessary
in MiniChronyd::Start(). I also found that running the scenario with
HMS enabled would fail as well, so I updated MiniHms::Start()
accordingly.
In addition, I updated the ExternalMiniClusterTest.TestBasicOperation
test scenario to cover the updated functionality.
Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
---
M src/kudu/clock/test/mini_chronyd.cc
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
4 files changed, 129 insertions(+), 102 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/14345/1
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14345
to look at the new patch set (#4).
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
[mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
This patch makes it possible to start ExternalMiniCluster on a directory
structure originally created by an earlier ExternalMiniCluster, provided
the same set of master RPC endpoints are used in both cases.
This functionality is useful for test scenarios like
MasterMigrationTest.TestEndToEndMigration, when a Kudu master catalog
is preserved between cluster's restarts.
The motivation for this patch was seeing the mentioned test scenario
failing if running with the built-in NTP. That's because
ExternalMiniCluster does the ephemeral port binding, and then hands off
the port to mini_chronyd. Since the port binding is ephemeral, next
start usually gets different port bound. However, upon next start,
mini_chronyd wasn't recreating the configuration file for chronyd
as necessary in MiniChronyd::Start(). I also found that running
the scenario with HMS enabled would fail as well, so I updated
MiniHms::Start() accordingly.
In addition, I updated the ExternalMiniClusterTest.TestBasicOperation
test scenario to cover the updated functionality.
Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
---
M src/kudu/clock/test/mini_chronyd.cc
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
4 files changed, 136 insertions(+), 102 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/14345/4
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14345 )
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14345/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14345/3//COMMIT_MSG@9
PS3, Line 9: This patch makes it possible to start ExternalMiniCluster on a directory
: structure created by its prior instance if using the masters' RPC
: end-points from the very first run.
> Maybe clearer as: "This patch makes it possible to start ExternalMiniCluste
Thanks!
Done.
http://gerrit.cloudera.org:8080/#/c/14345/3//COMMIT_MSG@18
PS3, Line 18: mini_chronyd wasn't recreating
: the configuration file for chronyd (the test NTP server) as necessary
: in MiniChronyd::Start().
> FWIW, this explanation is a bit confusing because, based on our earlier dis
I updated the explanation. Hopefully it's clearer now.
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/hms/mini_hms.cc@161
PS3, Line 161: // Run the schematool to initialize the database if not yet initialized.
> Maybe we can figure this out another way? Looks like the Derby database get
This sounds like a good idea, thanks. Done.
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/mini-cluster/external_mini_cluster-test.cc
File src/kudu/mini-cluster/external_mini_cluster-test.cc:
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@290
PS3, Line 290: // Shutdown the cluster explicitly.
: cluster->Shutdown();
> Maybe, we still need to test an explicit call of MiniClusterShutdown()? I'
OK, I added a comment that the call of it is redundant if thinking about shutting down cluster's compoments, but it's done just in the context of testing.
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 03 Oct 2019 17:07:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14345 )
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/14345/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14345/3//COMMIT_MSG@9
PS3, Line 9: This patch makes it possible to start ExternalMiniCluster on a directory
: structure created by its prior instance if using the masters' RPC
: end-points from the very first run.
Maybe clearer as: "This patch makes it possible to start ExternalMiniCluster on a directory structure originally created by an earlier ExternalMiniCluster, provided the same set of master RPC endpoints are used in both cases."
http://gerrit.cloudera.org:8080/#/c/14345/3//COMMIT_MSG@18
PS3, Line 18: mini_chronyd wasn't recreating
: the configuration file for chronyd (the test NTP server) as necessary
: in MiniChronyd::Start().
Why did we need to recreate the config file? Which properties changed?
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/hms/mini_hms.cc@161
PS3, Line 161: // Run the schematool to initialize the database if not yet initialized.
Does this tangibly slow down Start(), since it means starting yet another Java process?
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/mini-cluster/external_mini_cluster-test.cc
File src/kudu/mini-cluster/external_mini_cluster-test.cc:
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@290
PS3, Line 290: // Shutdown the cluster explicitly.
: cluster->Shutdown();
Could probably elide this; the test's destructor will do it.
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 02 Oct 2019 18:30:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14345 )
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
[mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
This patch makes it possible to start ExternalMiniCluster on a directory
structure originally created by an earlier ExternalMiniCluster, provided
the same set of master RPC endpoints are used in both cases.
This functionality is useful for test scenarios like
MasterMigrationTest.TestEndToEndMigration, when a Kudu master catalog
is preserved between cluster's restarts.
The motivation for this patch was seeing the mentioned test scenario
failing if running with the built-in NTP. That's because
ExternalMiniCluster does the ephemeral port binding, and then hands off
the port to mini_chronyd. Since the port binding is ephemeral, next
start usually gets different port bound. However, upon next start,
mini_chronyd wasn't recreating the configuration file for chronyd
as necessary in MiniChronyd::Start(). I also found that running
the scenario with HMS enabled would fail as well, so I updated
MiniHms::Start() accordingly.
In addition, I updated the ExternalMiniClusterTest.TestBasicOperation
test scenario to cover the updated functionality.
Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Reviewed-on: http://gerrit.cloudera.org:8080/14345
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/clock/test/mini_chronyd.cc
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
4 files changed, 141 insertions(+), 104 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14345
to look at the new patch set (#2).
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
[mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
This patch makes it possible to start ExternalMiniCluster on a directory
structure created by its prior instance if using the masters' RPC
end-points from the very first run.
This functionality is useful for test scenarios like
MasterMigrationTest.TestEndToEndMigration, when a Kudu master catalog
is preserved between cluster's restarts.
The motivation for this patch was seeing the mentioned test scenario
failed if running with the built-in NTP: mini_chronyd wasn't recreating
the configuration file for chronyd (the test NTP server) as necessary
in MiniChronyd::Start(). I also found that running the scenario with
HMS enabled would fail as well, so I updated MiniHms::Start()
accordingly.
In addition, I updated the ExternalMiniClusterTest.TestBasicOperation
test scenario to cover the updated functionality.
Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
---
M src/kudu/clock/test/mini_chronyd.cc
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
4 files changed, 130 insertions(+), 102 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/14345/2
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14345
to look at the new patch set (#5).
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
[mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
This patch makes it possible to start ExternalMiniCluster on a directory
structure originally created by an earlier ExternalMiniCluster, provided
the same set of master RPC endpoints are used in both cases.
This functionality is useful for test scenarios like
MasterMigrationTest.TestEndToEndMigration, when a Kudu master catalog
is preserved between cluster's restarts.
The motivation for this patch was seeing the mentioned test scenario
failing if running with the built-in NTP. That's because
ExternalMiniCluster does the ephemeral port binding, and then hands off
the port to mini_chronyd. Since the port binding is ephemeral, next
start usually gets different port bound. However, upon next start,
mini_chronyd wasn't recreating the configuration file for chronyd
as necessary in MiniChronyd::Start(). I also found that running
the scenario with HMS enabled would fail as well, so I updated
MiniHms::Start() accordingly.
In addition, I updated the ExternalMiniClusterTest.TestBasicOperation
test scenario to cover the updated functionality.
Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
---
M src/kudu/clock/test/mini_chronyd.cc
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
4 files changed, 142 insertions(+), 108 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/14345/5
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14345 )
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
Patch Set 6: Verified+1
Unrelated test failures:
* DiskFailure/DiskFailureITest.TestFailDuringServerStartup/0
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 03 Oct 2019 19:42:45 +0000
Gerrit-HasComments: No
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14345 )
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
Patch Set 4: Verified+1
Unrelated failure in tests:
* RaftConsensusITest.TestCommitIndexFarBehindAfterLeaderElection
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 03 Oct 2019 17:37:23 +0000
Gerrit-HasComments: No
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14345 )
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/14345/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14345/3//COMMIT_MSG@18
PS3, Line 18: mini_chronyd wasn't recreating
: the configuration file for chronyd (the test NTP server) as necessary
: in MiniChronyd::Start().
> Why did we need to recreate the config file? Which properties changed?
Since we use ephemeral port reservation technique for chronyd, next run of mini_chronyd from external minicluster will most likely get other port reserved.
I'll add these details.
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/hms/mini_hms.cc@161
PS3, Line 161: // Run the schematool to initialize the database if not yet initialized.
> Does this tangibly slow down Start(), since it means starting yet another J
Yep, it slows it down 1/3 (at 2GHz 16 code machine with a lot of memory and HDD it's about extra 5 seconds). It's also a concern to me.
We could get away fixing only MiniChronyd::Start(), and leaving MiniHms::Start() as is.
What do you think?
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/mini-cluster/external_mini_cluster-test.cc
File src/kudu/mini-cluster/external_mini_cluster-test.cc:
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/mini-cluster/external_mini_cluster-test.cc@290
PS3, Line 290: // Shutdown the cluster explicitly.
: cluster->Shutdown();
> Could probably elide this; the test's destructor will do it.
Maybe, we still need to test an explicit call of MiniClusterShutdown()? I'm not sure we want to remove it.
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 02 Oct 2019 23:54:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14345
to look at the new patch set (#6).
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
[mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
This patch makes it possible to start ExternalMiniCluster on a directory
structure originally created by an earlier ExternalMiniCluster, provided
the same set of master RPC endpoints are used in both cases.
This functionality is useful for test scenarios like
MasterMigrationTest.TestEndToEndMigration, when a Kudu master catalog
is preserved between cluster's restarts.
The motivation for this patch was seeing the mentioned test scenario
failing if running with the built-in NTP. That's because
ExternalMiniCluster does the ephemeral port binding, and then hands off
the port to mini_chronyd. Since the port binding is ephemeral, next
start usually gets different port bound. However, upon next start,
mini_chronyd wasn't recreating the configuration file for chronyd
as necessary in MiniChronyd::Start(). I also found that running
the scenario with HMS enabled would fail as well, so I updated
MiniHms::Start() accordingly.
In addition, I updated the ExternalMiniClusterTest.TestBasicOperation
test scenario to cover the updated functionality.
Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
---
M src/kudu/clock/test/mini_chronyd.cc
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
4 files changed, 141 insertions(+), 104 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/14345/6
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14345
to look at the new patch set (#3).
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
[mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
This patch makes it possible to start ExternalMiniCluster on a directory
structure created by its prior instance if using the masters' RPC
end-points from the very first run.
This functionality is useful for test scenarios like
MasterMigrationTest.TestEndToEndMigration, when a Kudu master catalog
is preserved between cluster's restarts.
The motivation for this patch was seeing the mentioned test scenario
failed if running with the built-in NTP: mini_chronyd wasn't recreating
the configuration file for chronyd (the test NTP server) as necessary
in MiniChronyd::Start(). I also found that running the scenario with
HMS enabled would fail as well, so I updated MiniHms::Start()
accordingly.
In addition, I updated the ExternalMiniClusterTest.TestBasicOperation
test scenario to cover the updated functionality.
Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
---
M src/kudu/clock/test/mini_chronyd.cc
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
4 files changed, 131 insertions(+), 102 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/14345/3
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14345 )
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14345/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14345/3//COMMIT_MSG@18
PS3, Line 18: mini_chronyd wasn't recreating
: the configuration file for chronyd (the test NTP server) as necessary
: in MiniChronyd::Start().
> Since we use ephemeral port reservation technique for chronyd, next run of
FWIW, this explanation is a bit confusing because, based on our earlier discussion, chronyd itself doesn't support ephemeral port binding. But I get what you're saying: ExternalMiniCluster does the ephemeral port binding, then hands off the port to chronyd.
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/14345/3/src/kudu/hms/mini_hms.cc@161
PS3, Line 161: // Run the schematool to initialize the database if not yet initialized.
> Yep, it slows it down 1/3 (at 2GHz 16 code machine with a lot of memory and
Maybe we can figure this out another way? Looks like the Derby database gets written to <data_root_>/warehouse. Could we check if that directory exists, and if so, skip the call to schematool?
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 03 Oct 2019 00:08:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14345 )
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14345/4/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/14345/4/src/kudu/hms/mini_hms.cc@165
PS4, Line 165: if (!Env::Default()->FileExists(JoinPathSegments(data_root_, "metadb"))) {
Looks like we plug this into the JDBC URL in the config file too. Could we consolidate behind a string constant so both sites can be changed with just one update?
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 03 Oct 2019 17:44:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14345 )
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 03 Oct 2019 18:36:03 +0000
Gerrit-HasComments: No
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14345 )
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
Patch Set 6: Code-Review+2
Carrying over Adar's +2 from PS5.
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 03 Oct 2019 19:43:06 +0000
Gerrit-HasComments: No
[kudu-CR] [mini cluster] small fix on Mini{Chronyd,Hms}::Start()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14345 )
Change subject: [mini_cluster] small fix on Mini{Chronyd,Hms}::Start()
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14345/4/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/14345/4/src/kudu/hms/mini_hms.cc@165
PS4, Line 165: if (!Env::Default()->FileExists(JoinPathSegments(data_root_, "metadb"))) {
> Looks like we plug this into the JDBC URL in the config file too. Could we
Whoops, I didn't notice that. Thanks!
Done.
--
To view, visit http://gerrit.cloudera.org:8080/14345
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I014eac84386c4d6e4f025afc1082e6a229a97454
Gerrit-Change-Number: 14345
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 03 Oct 2019 18:24:46 +0000
Gerrit-HasComments: Yes