[kudu-CR] [tests] clean-up on kudu::client::ScanToStrings
Alexey Serbin has uploaded a new change for review. http://gerrit.cloudera.org:8080/7027 Change subject: [tests] clean-up on kudu::client::ScanToStrings .. [tests] clean-up on kudu::client::ScanToStrings With this patch, kudu::client::ScanToStrings() returns Status. The new signature is more convenient for localizing scan errors, if any. Change-Id: I8e7ba56d2e946c1ac9a4db2ef3a542f0a118471d --- M src/kudu/client/client-test-util.cc M src/kudu/client/client-test-util.h M src/kudu/client/client-test.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/flex_partitioning-itest.cc 7 files changed, 38 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/7027/1 -- To view, visit http://gerrit.cloudera.org:8080/7027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I8e7ba56d2e946c1ac9a4db2ef3a542f0a118471d Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin
[kudu-CR] Move CreateTableForTesting into registration-test
Alexey Serbin has posted comments on this change. Change subject: Move CreateTableForTesting into registration-test .. Patch Set 3: Code-Review+2 LGTM The only nit is unused master::GetTableLocationsRequestPB in registration-test.cc reported by the Tidy Bot. -- To view, visit http://gerrit.cloudera.org:8080/6961 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id72a37b2dee05db65f27c27d52b0f18dd5c489f2 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] [tests] clean-up on kudu::client::ScanToStrings
Todd Lipcon has posted comments on this change. Change subject: [tests] clean-up on kudu::client::ScanToStrings .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7027/1/src/kudu/client/client-test-util.h File src/kudu/client/client-test-util.h: Line 57: Status ScanToStrings(KuduScanner* scanner, std::vector* row_strings); mind adding WARN_UNUSED_RESULT here, just in case anyone is in the midst of rebasing other older patches that might not be checking the result? -- To view, visit http://gerrit.cloudera.org:8080/7027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e7ba56d2e946c1ac9a4db2ef3a542f0a118471d Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR](gh-pages) WIP: Kudu Consistency Blog Post Pt1
Andrew Wong has posted comments on this change. Change subject: WIP: Kudu Consistency Blog Post Pt1 .. Patch Set 3: (32 comments) Really appreciated explicit definitions of what and when things happen. I think the section about design decisions can be shortened a bit but will see after the rest of the post is in. mostly comments about wording, some about content http://gerrit.cloudera.org:8080/#/c/7019/3/_posts/2017-05-30-kudu-consistency-pt1.md File _posts/2017-05-30-kudu-consistency-pt1.md: Line 11: of how they all play together and what are our future plans. would be nice to also include what is discussed in Part 1. PS3, Line 17: users and requests : are executed by many different machines, how about, "...users, and must serve requests across numerous machines,..."? PS3, Line 20: how Kudu implements this "how these systems implement" PS3, Line 29: single thread "single-threaded" Line 45: ## Motivation behind the consistency design decisions in Kudu: Can we assume the reader understands what a tradeoff is? If we can, I think we can take out the first paragraph. Maybe relabels this "Motivation behind some key design decisions in Kudu" since these aren't specifically consistency design decisions The key points here I think are: 1. the overarching goal for Kudu is fast analytic queries 2. we make similar design choices as those made in the Spanner paper w.r.t writes 3. we targeted continuous inserts 4. we wanted to grants users flexibility of the consistency constraint PS3, Line 95: semantics "guarantees", "constraints"? PS3, Line 101: Kudu currently lacks support for atomic multi-row mutation operations (i.e. mutation : operations to more than one row in the same or different tablets, planned as a future feature) so, : for the write path, we’ll be talking about the consistency semantics of single row mutations. : In this context we’ll discuss Kudu’s properties more from a key/value store standpoint. On the : other hand Kudu is an analytical storage engine so, for the read path, we’ll also discuss the : semantics of large (multi-row) scans. This moves the discussion more into the field of traditional : DBMSs. These ingredients make for a non-traditional discussion that is not exactly apples-to-apples : with what the reader might be familiar with, but our hope is that it still provides valuable, or : at least interesting, insight. maybe consider leaving the write and read discussion to their respective sections, or move this entire highlighted portion to the previous section (and leave this section as just "Relevant architectural components") PS3, Line 113: Operation execution "Transaction driver", "TransactionDriver"? trying to have each bullet's title be the component name Line 117: replicated to follower replicas by a leader replica following the Raft consensus algorithm[ref]. ", and thus, each tablet owns its own Consensus module." PS3, Line 121: Timestamp assignment and propagation maybe "Timestamp and Clock"? Or even have a separate bullet for "Clock". PS3, Line 130: landed "been added to"? PS3, Line 130: A request is not considered “committed” until is has landed on : the WAL of a majority of replicas Consider inverting this sentence, since we're introducing a bunch of terms here anyway: "A request is considered "committed" when it has been appended to a majority of replicas' WALs and registered with MVCC as committed" (and then maybe put the definition for WAL after MVCC) PS3, Line 132: This component serves a dual purpose storing the original client : requests and how they mutated each tablet’s internal state after being applied how about: "This component serves two purposes: it stores the original logical client-requested op (i.e. inserts, updates, deletes), and the physical change to the tablet after the op is applied (i.e. flushes and compactions). PS3, Line 136: Scans take a snapshots "A scan will take a snapshot" PS3, Line 136: they start executing and use "it starts executing, and will use" PS3, Line 137: unfinished operations as a tablet is scanned "unfinished tablet operations as it progresses" PS3, Line 139: component remove PS3, Line 159: 2. Timestamp assignment Note that this is done by the leader PS3, Line 161: follow up posts "a later post" PS3, Line 161: now the reader can just think of : it "a timestamp can be thought of" PS3, Line 162: That timestamp This PS3, Line 163: related operations Would "any two operations" be accurate as well? Or does "related operations" refer to something specific (e.g. operations on the same machine, in the same config, etc.)? PS3, Line 164: Register the operation with MVCC super nit: for consistency with previous bullets: "Registration of operation wi
[kudu-CR] KUDU-1914 Add positive .htpasswd test case
Hello Hao Hao, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7026 to look at the new patch set (#2). Change subject: KUDU-1914 Add positive .htpasswd test case .. KUDU-1914 Add positive .htpasswd test case This commit adds a test to check if a proper user/password pair allows a user to connect successfully to a Web UI with .htpasswd support enabled. Change-Id: Ia743ed41b9d68f9beb94c607e18613d0530526be --- M src/kudu/security/test/test_pass.cc M src/kudu/security/test/test_pass.h M src/kudu/server/webserver-test.cc M src/kudu/util/curl_util.cc 4 files changed, 16 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/7026/2 -- To view, visit http://gerrit.cloudera.org:8080/7026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia743ed41b9d68f9beb94c607e18613d0530526be Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sam Okrent Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] disk failure: coordinate IO error handling
Andrew Wong has uploaded a new change for review. http://gerrit.cloudera.org:8080/7029 Change subject: disk failure: coordinate IO error handling .. disk failure: coordinate IO error handling I/O errors are spawned when calls to env functions fail. The response to these errors may vary and may span many layers of abstraction. For instance, if an EIO were triggered while writing a block, one response might be to shut down the tablet whose operation just failed. In order to do this without messing up the layering, a new entity, the FsErrorManager is added to coordinate error handling. Callbacks are registered with the FsErrorManager as responses to errors, and callers to at-risk functions trigger these callbacks as errors are spawned. Change-Id: Ib3169deada702c527b70fabf9d2223364f9a9ea6 --- M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h A src/kudu/fs/error_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs-test-util.h M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/master/sys_catalog.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 13 files changed, 233 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/7029/1 -- To view, visit http://gerrit.cloudera.org:8080/7029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib3169deada702c527b70fabf9d2223364f9a9ea6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong
[kudu-CR] disk failure: basic test coverage for disk failure
Andrew Wong has uploaded a new change for review. http://gerrit.cloudera.org:8080/7031 Change subject: disk failure: basic test coverage for disk failure .. disk failure: basic test coverage for disk failure This patch adds an EMC test that spawns three servers and triggers EIOs on two of them to fail two different tablets. With improper disk-failure-handling, this scenario alone would have been enough to leave the server with only a single copy of data, as the two servers with EIOs would have been shut down entirely. With proper disk-failure handling, this scenario would be salvagable, and data would be replicated on the remaining disks. Change-Id: I8ff63ec71ab718866484b9f3ec7264bc72ecfe97 --- M src/kudu/integration-tests/multidir-cluster-itest.cc M src/kudu/integration-tests/ts_itest-base.h M src/kudu/tablet/tablet_replica_mm_ops.cc 3 files changed, 137 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/7031/1 -- To view, visit http://gerrit.cloudera.org:8080/7031 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I8ff63ec71ab718866484b9f3ec7264bc72ecfe97 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong
[kudu-CR] disk failure: make DataDirManager failure-aware
Andrew Wong has uploaded a new change for review. http://gerrit.cloudera.org:8080/7028 Change subject: disk failure: make DataDirManager failure-aware .. disk failure: make DataDirManager failure-aware The DataDirManager must record what directories are unhealthy in order to avoid placing new data on failed disks. This patch achieves this by maintaining a set of UUID indices in the DataDirManager that correspond to failed directories. Tests are added to data_dirs-test to ensure that failed directories are not used and are not returned as part of newly created DataDirGroups. If no healthy directories exist, callers will return an IOError with posix code ENODEV. Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2 --- M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h 3 files changed, 126 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/7028/1 -- To view, visit http://gerrit.cloudera.org:8080/7028 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Iee212793152de5de5198751d649ab34fb97f6aa2 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong
[kudu-CR] [consensus queue] fix race in UpdateLagMetrics()
Alexey Serbin has uploaded a new change for review. http://gerrit.cloudera.org:8080/7032 Change subject: [consensus_queue] fix race in UpdateLagMetrics() .. [consensus_queue] fix race in UpdateLagMetrics() TSAN reports warnings on races on writing/reading the QueueState::last_idx_appended_to_leader field: CatalogManagerTskITest.LeadershipChangeOnTskGeneration: WARNING: ThreadSanitizer: data race (pid=2710) Write of size 8 at 0x7d53e480 by thread T33 (mutexes: write M1863, write M1821): #0 kudu::consensus::PeerMessageQueue::UpdateLastIndexAppendedToLeader(long) consensus/consensus_queue.cc:607:44 #1 kudu::consensus::RaftConsensus::UpdateReplica(kudu::consensus::ConsensusRequestPB const*, kudu::consensus::ConsensusResponsePB*) consensus/raft_consensus.cc:1155:13 #2 kudu::consensus::RaftConsensus::Update(kudu::consensus::ConsensusRequestPB const*, kudu::consensus::ConsensusResponsePB*) consensus/raft_consensus.cc:752:14 #3 kudu::tserver::ConsensusServiceImpl::UpdateConsensus(kudu::consensus::ConsensusRequestPB const*, kudu::consensus::ConsensusResponsePB*, kudu::rpc::RpcContext*) tserver/tablet_service.cc:861:25 #4 kudu::consensus::ConsensusServiceIf::ConsensusServiceIf(scoped_refptr const&, scoped_refptr const&)::$_1::operator()(google::protobuf::Message const*, google::protobuf::Message*, kudu::rpc::RpcContext*) const consensus/consensus.service.cc:100:13 ... skipped ... Previous read of size 8 at 0x7d53e480 by thread T79 (mutexes: write M1822): #0 kudu::consensus::PeerMessageQueue::UpdateLagMetrics() consensus/consensus_queue.cc:602:20 #1 kudu::consensus::PeerMessageQueue::UpdateMetrics() consensus/consensus_queue.cc:875:3 #2 kudu::consensus::PeerMessageQueue::ResponseFromPeer(std::__1::basic_string, std::__1::allocator > const&, kudu::consensus::ConsensusResponsePB const&, bool*) consensus/consensus_queue.cc:828:5 ... skipped ... For more details, see http://dist-test.cloudera.org:8080/diagnose?key=0784c33a-41b5-11e7-9f6b-0242ac11000f This patch fixes the above race. Also, it contains some extras: * The PeerMessageQueue::UpdateLagMetrics() method has been renamed into PeerMessageQueue::UpdateLagMetricsUnlocked() and made private. * The PeerMessageQueue::UpdateMetrics() method has been renamed into PeerMessageQueue::UpdateMetricsUnlocked(). * Added DCHECK(queue_lock_.is_locked()) into all PeerMessageQueue::XxxUnlocked() methods. Change-Id: I25feb676619cc1f3a94fb8e631bffd8ca02ead49 --- M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/consensus_queue.h 2 files changed, 24 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/32/7032/1 -- To view, visit http://gerrit.cloudera.org:8080/7032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I25feb676619cc1f3a94fb8e631bffd8ca02ead49 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin
[kudu-CR] KUDU-1125 (part 1) catalog manager: try to avoid unnecessarily rewriting tablet info
Todd Lipcon has submitted this change and it was merged. Change subject: KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info .. KUDU-1125 (part 1) catalog_manager: try to avoid unnecessarily rewriting tablet info This adds some checks in SysCatalogTable so that, if the updated version of a catalog table entry is identical to the previous version, we avoid writing anything to the table. This is a simple way of short-circuiting a common case of unnecessary slowness in tablet report processing. For example, when the master restarts or fails over, all of the tablet servers perform a full tablet report. However, most of the tablets will not have changed any information since their prior report, in which case the writes can be safely skipped on the master. This doesn't achieve all of the goals of KUDU-1125 (we still do separate sync writes for each _changed_ reported tablet) but should still be a substantial reduction. Perhaps most importantly, if a heartbeat from a tserver times out due to long processing, the retry from the TS should hit this code path and therefore be processed quickly. Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba Reviewed-on: http://gerrit.cloudera.org:8080/6916 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/integration-tests/registration-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master-test-util.h M src/kudu/master/sys_catalog.cc M src/kudu/master/sys_catalog.h 5 files changed, 106 insertions(+), 17 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I0de7189b8f1dbeea55172929396b73fd92fd62ba Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] disk failure: handle EIOs on I/O to blocks
Andrew Wong has uploaded a new change for review. http://gerrit.cloudera.org:8080/7030 Change subject: disk failure: handle EIOs on I/O to blocks .. disk failure: handle EIOs on I/O to blocks This patch adds the proper macros around operations that touch disk that occur when writing or reading blocks. Change-Id: Ia03bfb711a1b022d7516f4adb37fe9fb28ec949c --- M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_writer.cc M src/kudu/fs/file_block_manager.cc M src/kudu/fs/log_block_manager.cc M src/kudu/util/status.h 5 files changed, 45 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/7030/1 -- To view, visit http://gerrit.cloudera.org:8080/7030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ia03bfb711a1b022d7516f4adb37fe9fb28ec949c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong
[kudu-CR] Move CreateTableForTesting into registration-test
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6961 to look at the new patch set (#4). Change subject: Move CreateTableForTesting into registration-test .. Move CreateTableForTesting into registration-test This utility function was only used in one spot, so moving it out of the header. Change-Id: Id72a37b2dee05db65f27c27d52b0f18dd5c489f2 --- M src/kudu/integration-tests/registration-test.cc M src/kudu/master/master-test-util.h 2 files changed, 69 insertions(+), 59 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/6961/4 -- To view, visit http://gerrit.cloudera.org:8080/6961 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id72a37b2dee05db65f27c27d52b0f18dd5c489f2 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus: Get rid of ReplicaState class
Todd Lipcon has posted comments on this change. Change subject: consensus: Get rid of ReplicaState class .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7007 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie1e62eff37d3f8655100b364939375608063aa80 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Move CreateTableForTesting into registration-test
Alexey Serbin has posted comments on this change. Change subject: Move CreateTableForTesting into registration-test .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6961 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id72a37b2dee05db65f27c27d52b0f18dd5c489f2 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] consensus: Get rid of LockFor*() methods
Todd Lipcon has posted comments on this change. Change subject: consensus: Get rid of LockFor*() methods .. Patch Set 10: think you missed a couple comments on the previous rev? -- To view, visit http://gerrit.cloudera.org:8080/7012 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] consensus: Add DCHECK(is locked()) in all unlocked methods
Todd Lipcon has posted comments on this change. Change subject: consensus: Add DCHECK(is_locked()) in all unlocked methods .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3364607e6778e6495e40d7ad36ea1850b022c444 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [tests] clean-up on kudu::client::ScanToStrings
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7027 to look at the new patch set (#2). Change subject: [tests] clean-up on kudu::client::ScanToStrings .. [tests] clean-up on kudu::client::ScanToStrings With this patch, kudu::client::ScanToStrings() returns Status. The new signature is more convenient for localizing scan errors, if any. Change-Id: I8e7ba56d2e946c1ac9a4db2ef3a542f0a118471d --- M src/kudu/client/client-test-util.cc M src/kudu/client/client-test-util.h M src/kudu/client/client-test.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/flex_partitioning-itest.cc 7 files changed, 41 insertions(+), 41 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/7027/2 -- To view, visit http://gerrit.cloudera.org:8080/7027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8e7ba56d2e946c1ac9a4db2ef3a542f0a118471d Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1914 Add positive .htpasswd test case
Todd Lipcon has submitted this change and it was merged. Change subject: KUDU-1914 Add positive .htpasswd test case .. KUDU-1914 Add positive .htpasswd test case This commit adds a test to check if a proper user/password pair allows a user to connect successfully to a Web UI with .htpasswd support enabled. Change-Id: Ia743ed41b9d68f9beb94c607e18613d0530526be Reviewed-on: http://gerrit.cloudera.org:8080/7026 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon --- M src/kudu/security/test/test_pass.cc M src/kudu/security/test/test_pass.h M src/kudu/server/webserver-test.cc M src/kudu/util/curl_util.cc 4 files changed, 16 insertions(+), 4 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ia743ed41b9d68f9beb94c607e18613d0530526be Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sam Okrent Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [tests] clean-up on kudu::client::ScanToStrings
Alexey Serbin has posted comments on this change. Change subject: [tests] clean-up on kudu::client::ScanToStrings .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7027/1/src/kudu/client/client-test-util.h File src/kudu/client/client-test-util.h: Line 57: Status ScanToStrings(KuduScanner* scanner, std::vector* row_strings); > mind adding WARN_UNUSED_RESULT here, just in case anyone is in the midst of It's a good idea, thanks. -- To view, visit http://gerrit.cloudera.org:8080/7027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e7ba56d2e946c1ac9a4db2ef3a542f0a118471d Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1914 Add positive .htpasswd test case
Todd Lipcon has posted comments on this change. Change subject: KUDU-1914 Add positive .htpasswd test case .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7026 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia743ed41b9d68f9beb94c607e18613d0530526be Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sam Okrent Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [tests] InvalidTokenDuringSeparateWorkloads test
Todd Lipcon has posted comments on this change. Change subject: [tests] InvalidTokenDuringSeparateWorkloads test .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] consensus: Add DCHECK(is locked()) in all unlocked methods
Alexey Serbin has posted comments on this change. Change subject: consensus: Add DCHECK(is_locked()) in all unlocked methods .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7013 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3364607e6778e6495e40d7ad36ea1850b022c444 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [tests] clean-up on kudu::client::ScanToStrings
Todd Lipcon has posted comments on this change. Change subject: [tests] clean-up on kudu::client::ScanToStrings .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e7ba56d2e946c1ac9a4db2ef3a542f0a118471d Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] consensus: Get rid of LockFor*() methods
Mike Percy has posted comments on this change. Change subject: consensus: Get rid of LockFor*() methods .. Patch Set 10: > think you missed a couple comments on the previous rev? Ah, I missed your comments in the churn. Will take a look. -- To view, visit http://gerrit.cloudera.org:8080/7012 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [consensus queue] fix race in UpdateLagMetrics()
Todd Lipcon has posted comments on this change. Change subject: [consensus_queue] fix race in UpdateLagMetrics() .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7032/1/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: Line 603: std::lock_guard l(queue_lock_); I think this is OK, but just to confirm... this is called from RaftConsensus while holding RaftConsensus's lock. Is that the correct ordering? RaftConsensus -> ConsensusQueue? Do we ever invert that, leading to possible deadlock? (I don't think so based on me reading, but curious if you also looked) -- To view, visit http://gerrit.cloudera.org:8080/7032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I25feb676619cc1f3a94fb8e631bffd8ca02ead49 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] disk failure: coordinate IO error handling
Andrew Wong has posted comments on this change. Change subject: disk failure: coordinate IO error handling .. Patch Set 1: Definitely open to feedback on the observer pattern-ish design -- To view, visit http://gerrit.cloudera.org:8080/7029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib3169deada702c527b70fabf9d2223364f9a9ea6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [consensus queue] fix race in UpdateLagMetrics()
Alexey Serbin has posted comments on this change. Change subject: [consensus_queue] fix race in UpdateLagMetrics() .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7032/1/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: Line 603: std::lock_guard l(queue_lock_); > I think this is OK, but just to confirm... this is called from RaftConsensu I took a look at the places where PeerMessageQueue::UpdateLastIndexAppendedToLeader() is called and I didn't find any suspicious places. Actually, the only non-test place it's called is in RaftConsensus::UpdateReplica(). I also found that many methods like RaftConsensus::XxxUnlocked() call Queue methods which take the same lock, so I think it should be fine. Otherwise, I was thinking, would use atomic instead. I also ran consensus-related tests in many iterations via dist-test, and no issues were reported (at least in DEBUG build). -- To view, visit http://gerrit.cloudera.org:8080/7032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I25feb676619cc1f3a94fb8e631bffd8ca02ead49 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [tests] InvalidTokenDuringSeparateWorkloads test
Hello Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7025 to look at the new patch set (#2). Change subject: [tests] InvalidTokenDuringSeparateWorkloads test .. [tests] InvalidTokenDuringSeparateWorkloads test Introduced new AuthnTokenExpireITest.InvalidTokenDuringSeparateWorkloads test to cover authn token re-acquiring for both read-only and write-only workloads. This is to stay clear from the implementation details of the TestWorkload class: it uses the same client object for both the read and the write operations, while the authn token is stored as a shared piece of data in the client object. Also, renamed AuthnTokenExpireITest.InvalidTokenDuringWorkload test into AuthnTokenExpireITest.InvalidTokenDuringMixedWorkload. Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2 --- M src/kudu/integration-tests/authn_token_expire-itest.cc 1 file changed, 66 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/7025/2 -- To view, visit http://gerrit.cloudera.org:8080/7025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [consensus queue] fix race in UpdateLagMetrics()
Andrew Wong has posted comments on this change. Change subject: [consensus_queue] fix race in UpdateLagMetrics() .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7032/1/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: Line 603: std::lock_guard l(queue_lock_); > I took a look at the places where PeerMessageQueue::UpdateLastIndexAppended Right, that should be the only call in a non-test. Hrm, I thought metrics were by default atomic, at least from a brief look in metrics.h. But we would still need locking here to prevent the race. -- To view, visit http://gerrit.cloudera.org:8080/7032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I25feb676619cc1f3a94fb8e631bffd8ca02ead49 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [tests] InvalidTokenDuringSeparateWorkloads test
Alexey Serbin has posted comments on this change. Change subject: [tests] InvalidTokenDuringSeparateWorkloads test .. Patch Set 2: > Uploaded patch set 2. Needed to re-base because of merge conflicts with f5e0024e07645841fafabb8eae8351872516e79d -- To view, visit http://gerrit.cloudera.org:8080/7025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [consensus queue] fix race in UpdateLagMetrics()
Alexey Serbin has posted comments on this change. Change subject: [consensus_queue] fix race in UpdateLagMetrics() .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7032/1/src/kudu/consensus/consensus_queue.cc File src/kudu/consensus/consensus_queue.cc: Line 603: std::lock_guard l(queue_lock_); > Right, that should be the only call in a non-test. Yep, the not-so-obvious thing was accessing that last_idx_appended_to_leader from multiple places, and that's exactly where the race happened. -- To view, visit http://gerrit.cloudera.org:8080/7032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I25feb676619cc1f3a94fb8e631bffd8ca02ead49 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Move CreateTableForTesting into registration-test
Todd Lipcon has submitted this change and it was merged. Change subject: Move CreateTableForTesting into registration-test .. Move CreateTableForTesting into registration-test This utility function was only used in one spot, so moving it out of the header. Change-Id: Id72a37b2dee05db65f27c27d52b0f18dd5c489f2 Reviewed-on: http://gerrit.cloudera.org:8080/6961 Reviewed-by: Alexey Serbin Tested-by: Kudu Jenkins --- M src/kudu/integration-tests/registration-test.cc M src/kudu/master/master-test-util.h 2 files changed, 69 insertions(+), 59 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6961 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Id72a37b2dee05db65f27c27d52b0f18dd5c489f2 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [tests] clean-up on kudu::client::ScanToStrings
Alexey Serbin has submitted this change and it was merged. Change subject: [tests] clean-up on kudu::client::ScanToStrings .. [tests] clean-up on kudu::client::ScanToStrings With this patch, kudu::client::ScanToStrings() returns Status. The new signature is more convenient for localizing scan errors, if any. Change-Id: I8e7ba56d2e946c1ac9a4db2ef3a542f0a118471d Reviewed-on: http://gerrit.cloudera.org:8080/7027 Reviewed-by: Todd Lipcon Tested-by: Kudu Jenkins --- M src/kudu/client/client-test-util.cc M src/kudu/client/client-test-util.h M src/kudu/client/client-test.cc M src/kudu/integration-tests/alter_table-randomized-test.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/flex_partitioning-itest.cc 7 files changed, 41 insertions(+), 41 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7027 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I8e7ba56d2e946c1ac9a4db2ef3a542f0a118471d Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [tests] InvalidTokenDuringSeparateWorkloads test
Alexey Serbin has posted comments on this change. Change subject: [tests] InvalidTokenDuringSeparateWorkloads test .. Patch Set 2: Code-Review+2 carrying over Todd's +2 -- To view, visit http://gerrit.cloudera.org:8080/7025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [tests] InvalidTokenDuringSeparateWorkloads test
Hello Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7025 to look at the new patch set (#3). Change subject: [tests] InvalidTokenDuringSeparateWorkloads test .. [tests] InvalidTokenDuringSeparateWorkloads test Introduced new AuthnTokenExpireITest.InvalidTokenDuringSeparateWorkloads test to cover authn token re-acquiring for both read-only and write-only workloads. This is to stay clear from the implementation details of the TestWorkload class: it uses the same client object for both the read and the write operations, while the authn token is stored as a shared piece of data in the client object. Also, renamed AuthnTokenExpireITest.InvalidTokenDuringWorkload test into AuthnTokenExpireITest.InvalidTokenDuringMixedWorkload. Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2 --- M src/kudu/integration-tests/authn_token_expire-itest.cc 1 file changed, 68 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/7025/3 -- To view, visit http://gerrit.cloudera.org:8080/7025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [tests] InvalidTokenDuringSeparateWorkloads test
Todd Lipcon has posted comments on this change. Change subject: [tests] InvalidTokenDuringSeparateWorkloads test .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] consensus: Get rid of ReplicaState class
Mike Percy has posted comments on this change. Change subject: consensus: Get rid of ReplicaState class .. Patch Set 6: Verified+1 Overriding Spark test cleanup issue: org.apache.kudu.client.NonRecoverableException: Table startswith already exists with id d53754f87fda4558bbaedc428a40755d -- To view, visit http://gerrit.cloudera.org:8080/7007 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie1e62eff37d3f8655100b364939375608063aa80 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] consensus: Get rid of ReplicaState class
Mike Percy has submitted this change and it was merged. Change subject: consensus: Get rid of ReplicaState class .. consensus: Get rid of ReplicaState class Merges the logic in ReplicaState into the RaftConsensus class. ReplicaState adds complexity but doesn't really serve a purpose anymore. There are no functional changes in this patch. Change-Id: Ie1e62eff37d3f8655100b364939375608063aa80 Reviewed-on: http://gerrit.cloudera.org:8080/7007 Reviewed-by: Todd Lipcon Tested-by: Mike Percy --- M src/kudu/consensus/CMakeLists.txt M src/kudu/consensus/consensus.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_quorum-test.cc D src/kudu/consensus/raft_consensus_state.cc D src/kudu/consensus/raft_consensus_state.h 7 files changed, 700 insertions(+), 876 deletions(-) Approvals: Mike Percy: Verified Todd Lipcon: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/7007 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ie1e62eff37d3f8655100b364939375608063aa80 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [tests] InvalidTokenDuringSeparateWorkloads test
Alexey Serbin has submitted this change and it was merged. Change subject: [tests] InvalidTokenDuringSeparateWorkloads test .. [tests] InvalidTokenDuringSeparateWorkloads test Introduced new AuthnTokenExpireITest.InvalidTokenDuringSeparateWorkloads test to cover authn token re-acquiring for both read-only and write-only workloads. This is to stay clear from the implementation details of the TestWorkload class: it uses the same client object for both the read and the write operations, while the authn token is stored as a shared piece of data in the client object. Also, renamed AuthnTokenExpireITest.InvalidTokenDuringWorkload test into AuthnTokenExpireITest.InvalidTokenDuringMixedWorkload. Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2 Reviewed-on: http://gerrit.cloudera.org:8080/7025 Reviewed-by: Todd Lipcon Tested-by: Kudu Jenkins --- M src/kudu/integration-tests/authn_token_expire-itest.cc 1 file changed, 68 insertions(+), 3 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I7982dfd8456791e57663fc05664e40ff205c42a2 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2027 retry scan RPC if negotiation times out
Alexey Serbin has uploaded a new change for review. http://gerrit.cloudera.org:8080/7037 Change subject: KUDU-2027 retry scan RPC if negotiation times out .. KUDU-2027 retry scan RPC if negotiation times out This patch addresses KUDU-2027, i.e. with this patch the Kudu C++ client retries a scan RPC with other tablet replica if the connection negotiation with current replica timed out. Added new integration test to cover the updated client's behavior. This patch is a follow-up for e3c5dd18c22b9e20358f05dcd301e7736c0a3321, since KUDU-2027 is a scan-related counterpart of KUDU-1580. Change-Id: I05a3c38cc07c9b6edd1ae773d9850e92645f3b14 --- M src/kudu/client/scanner-internal.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc 2 files changed, 93 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/7037/1 -- To view, visit http://gerrit.cloudera.org:8080/7037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I05a3c38cc07c9b6edd1ae773d9850e92645f3b14 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin
[kudu-CR] consensus: Get rid of LockFor*() methods
Alexey Serbin has posted comments on this change. Change subject: consensus: Get rid of LockFor*() methods .. Patch Set 10: (7 comments) http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: PS10, Line 284: << "Replica is not in kInitialized state: " nit: I would drop this part since it does not make the message more actionable. PS10, Line 313: } : : { : ThreadRestrictions::AssertWaitAllowed(); : LockGuard l(lock_); It seems this could be safely removed. PS10, Line 1360: SnoozeFailureDetectorUnlocked() What if it returns non-OK status? Should that case be handled somehow? PS10, Line 2120: UniqueLock why not LockGuard? PS10, Line 2424: RETURN_NOT_OK(CheckActiveLeaderUnlocked()); : return Status::OK(); nit: this could be reduced to return CheckActiveLeaderUnlocked(); http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: PS10, Line 225: using UniqueLock = std::unique_lock; I found only one place which uses the UniqueLock in the .cc file, and I'm not sure that's necessary. Consider dropping this. PS10, Line 469: operation message? -- To view, visit http://gerrit.cloudera.org:8080/7012 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2027 retry scan RPC if negotiation times out
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7037 to look at the new patch set (#2). Change subject: KUDU-2027 retry scan RPC if negotiation times out .. KUDU-2027 retry scan RPC if negotiation times out This patch addresses KUDU-2027, i.e. with this patch the Kudu C++ client retries a scan RPC with other tablet replica if the connection negotiation with current replica timed out. Added new integration test to cover the updated client's behavior. This patch is a follow-up for e3c5dd18c22b9e20358f05dcd301e7736c0a3321, since KUDU-2027 is a scan-related counterpart of KUDU-1580. Change-Id: I05a3c38cc07c9b6edd1ae773d9850e92645f3b14 --- M src/kudu/client/scanner-internal.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc 2 files changed, 95 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/7037/2 -- To view, visit http://gerrit.cloudera.org:8080/7037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I05a3c38cc07c9b6edd1ae773d9850e92645f3b14 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] consensus: Get rid of LockFor*() methods
Mike Percy has posted comments on this change. Change subject: consensus: Get rid of LockFor*() methods .. Patch Set 10: (11 comments) http://gerrit.cloudera.org:8080/#/c/7012/8/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: PS8, Line 718: wers is disabled. Doing nothing."; > this message could be updated now Done Line 1435: > seems like everywhere you take the lock, you are using AssertWaitAllowed. I would rather do that in a follow-up patch since this patch is only intended to move code around without changing functionality. Right now we use a spinlock, not a mutex. http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: PS10, Line 284: << "Replica is not in kInitialized state: " > nit: I would drop this part since it does not make the message more actiona Done PS10, Line 313: } : : { : ThreadRestrictions::AssertWaitAllowed(); : LockGuard l(lock_); > It seems this could be safely removed. I would rather do that in a follow-up patch since this patch is not intended to change any functionality. PS10, Line 1360: SnoozeFailureDetectorUnlocked() > What if it returns non-OK status? Should that case be handled somehow? Nice catch, done. PS10, Line 2120: UniqueLock > why not LockGuard? We need to unlock() below at L2178 Line 2178: lock.unlock(); We unlock here. PS10, Line 2424: RETURN_NOT_OK(CheckActiveLeaderUnlocked()); : return Status::OK(); > nit: this could be reduced to Done http://gerrit.cloudera.org:8080/#/c/7012/8/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: PS8, Line 479: > this should probably also have WARN_UNUSED_RESULT Done http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: PS10, Line 225: using UniqueLock = std::unique_lock; > I found only one place which uses the UniqueLock in the .cc file, and I'm n see my reply on the other comment PS10, Line 469: operation > message? I like the terminology of Operation better, since we are replicating things like Write operations. See OperationType in consensus.proto -- To view, visit http://gerrit.cloudera.org:8080/7012 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] consensus: Get rid of LockFor*() methods
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7012 to look at the new patch set (#11). Change subject: consensus: Get rid of LockFor*() methods .. consensus: Get rid of LockFor*() methods Simplify the locking logic by removing layers of abstraction. Also add State_Name() helper for state-related error messages. Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71 --- M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_quorum-test.cc 3 files changed, 170 insertions(+), 207 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/7012/11 -- To view, visit http://gerrit.cloudera.org:8080/7012 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] consensus: Get rid of LockFor*() methods
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7012 to look at the new patch set (#12). Change subject: consensus: Get rid of LockFor*() methods .. consensus: Get rid of LockFor*() methods Simplify the locking logic by removing layers of abstraction. Also add State_Name() helper for state-related error messages. Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71 --- M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_quorum-test.cc 3 files changed, 170 insertions(+), 207 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/7012/12 -- To view, visit http://gerrit.cloudera.org:8080/7012 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] consensus: Get rid of LockFor*() methods
Alexey Serbin has posted comments on this change. Change subject: consensus: Get rid of LockFor*() methods .. Patch Set 12: Code-Review+2 (3 comments) LGTM. You might be interested to get more feedback from Todd. http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: PS10, Line 284:<< State_Name(st > Done agreed -- it's better to make small verifiable steps PS10, Line 2120: UniqueLock > We need to unlock() below at L2178 woops, I missed that. http://gerrit.cloudera.org:8080/#/c/7012/10/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: PS10, Line 225: using UniqueLock = std::unique_lock; > see my reply on the other comment Yep, thanks -- it seems I missed the unlock() part. -- To view, visit http://gerrit.cloudera.org:8080/7012 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6858752f4fbeb70b09eb4375c52e4aeaa1bb8e71 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tablet: Get rid of TabletStatusListener
Hello Todd Lipcon, Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7039 to review the following change. Change subject: tablet: Get rid of TabletStatusListener .. tablet: Get rid of TabletStatusListener There is no need for this interface. We don't have a circular ownership dependency related to TabletReplica. Change-Id: I92204ec9d6f5db218deb51c83e44f821e2ed2442 --- M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/tablet_bootstrap.h M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 8 files changed, 25 insertions(+), 41 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7039/1 -- To view, visit http://gerrit.cloudera.org:8080/7039 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I92204ec9d6f5db218deb51c83e44f821e2ed2442 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Todd Lipcon
[kudu-CR] consensus: Remove Consensus interface
Hello Todd Lipcon, Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7040 to review the following change. Change subject: consensus: Remove Consensus interface .. consensus: Remove Consensus interface We only have one Consensus implementation now, and have no plans for additional implementations in the future. So we can remove this interface. Change-Id: I21b976cb92619083e1f1766b13634b841b554c8c --- M src/kudu/consensus/CMakeLists.txt M src/kudu/consensus/consensus-test-util.h D src/kudu/consensus/consensus.cc D src/kudu/consensus/consensus.h M src/kudu/consensus/leader_election.h M src/kudu/consensus/pending_rounds.cc M src/kudu/consensus/pending_rounds.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/raft_consensus_quorum-test.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tablet/transactions/transaction.h M src/kudu/tablet/transactions/transaction_driver.cc M src/kudu/tablet/transactions/transaction_driver.h M src/kudu/tablet/transactions/write_transaction.h M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/tserver-path-handlers.cc 23 files changed, 446 insertions(+), 617 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/7040/1 -- To view, visit http://gerrit.cloudera.org:8080/7040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I21b976cb92619083e1f1766b13634b841b554c8c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Todd Lipcon
[kudu-CR] external minicluster: expand EMC dir usage
Mike Percy has posted comments on this change. Change subject: external minicluster: expand EMC dir usage .. Patch Set 16: (6 comments) http://gerrit.cloudera.org:8080/#/c/6845/16/src/kudu/integration-tests/multidir-cluster-itest.cc File src/kudu/integration-tests/multidir-cluster-itest.cc: Line 25: #include "kudu/integration-tests/ts_itest-base.h" nit: alpha order PS16, Line 40: TabletServerIntegrationTestBase Would you mind using ExternalMiniClusterITestBase for this test? It's what we've been standardizing on for more recent itests. Line 83: NO_FATALS(CreateTable()); You can use TestWorkload.Setup() to easily create a table Line 103: InsertPayload(kTableId, 0, 10, 10); You should be able to get the same effect using TestWorkload.Start() and StopAndJoin() after the ASSERT_EVENTUALLY Line 104: SleepFor(kTimeout); This is potentially flaky. See below for a suggestion. After making those changes, would you mind running 200 iterations of this test using dist_test.py with --stress_cpu_threads=8 and post the link to the results? LMK if you need help to get that set up and I can show you how to do it. PS16, Line 107: int num_dirs_added_to = 0; : for (const string& data_dir : ext_tserver->data_dirs()) { : string data_path = JoinPathSegments(data_dir, "data"); : vector files; : inspect_->ListFilesInDir(data_path, &files); : int* num_files_before_insert = FindOrNull(num_files_in_each_dir, data_dir); : ASSERT_NE(nullptr, num_files_before_insert); : if (*num_files_before_insert < files.size()) { : num_dirs_added_to++; : } : } : // Block placement should guarantee that more than one data dir will have : // data written to it. : ASSERT_GT(num_dirs_added_to, 1); You can wrap this in ASSERT_EVENTUALLY() and avoid the potentially-flaky SleepFor() above -- To view, visit http://gerrit.cloudera.org:8080/6845 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id2f5def6980ad394c8558ad97ba830f1b0257332 Gerrit-PatchSet: 16 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes