[kudu-CR] KUDU-2463 pt 2: bump MVCC safe time on Raft no-op
Hello Tidy Bot, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11427 to look at the new patch set (#3). Change subject: KUDU-2463 pt 2: bump MVCC safe time on Raft no-op .. KUDU-2463 pt 2: bump MVCC safe time on Raft no-op Based on the same rationale as Part 1 of this patch series, this patch updates MVCC's safe and clean time using the no-op timestamp provided by the leader following a successful Raft election. There isn't an obvious reference to the tablet (to get to the MVCC module) in Raft consensus, but there is a ReplicaTransactionFactory, that the TabletReplica implements. I've extended this to be a more general ConsensusRoundHandler that can be used to create transactions or finish transactions as needed. One thing to note is that in some cases (e.g. brand new tablets), the first election would replicate the no-op with a timestamp of 1 (the timestamp we're trying to avoid). I tracked the cause of this to be the fact that sometimes the hybrid clock doesn't get updated before sending out the first no-op, and this will result in the first assigned timestamp being 1. To work around this, I updated the clock to the initial clean time, which seems in line with other updates to the hybrid clock in the time manager. I tested this in the following ways: - to ensure nothing terrible happens when there is a lot of election churn (and hence, a lot of timestamp advancement), I've tweaked exactly_once_writes-itest to actually churn elections. Previously it attempted this with just a low timeout; I injected some latency to make it churn a bit harder. - I added a test that ensured that, on its own, a tablet would bump its MVCC timestamps, by virtue of its elections - I tested the above with single-replica tablets as well - a few other tests needed to be tweaked given the extra bump to the hybrid clock This patch alone doesn't fix KUDU-2463. Rather, a later patch will prevent scans from occuring if the MVCC safe time hasn't been advanced, at which point this patch will reduce the window of scan unavailability. Change-Id: Icbf812e2cb7c322fd980245cfe40c886a15a --- M src/kudu/consensus/consensus-test-util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/time_manager.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/exactly_once_writes-itest.cc M src/kudu/integration-tests/raft_consensus-itest.cc M src/kudu/integration-tests/timestamp_advancement-itest.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h 10 files changed, 180 insertions(+), 78 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/11427/3 -- To view, visit http://gerrit.cloudera.org:8080/11427 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icbf812e2cb7c322fd980245cfe40c886a15a Gerrit-Change-Number: 11427 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2463 pt 1: adjust MVCC when replaying no-ops
Hello Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11142 to look at the new patch set (#7). Change subject: KUDU-2463 pt 1: adjust MVCC when replaying no-ops .. KUDU-2463 pt 1: adjust MVCC when replaying no-ops Previously, during tablet bootstrap, a tablet replica would only update its MVCC safetime based on write and alter schema messages, as the timestamps in these messages are guaranteed to be serialized with respect to one another, by virtue of being assigned in a single thread (the prepare thread) on the leader replica. From this, we conclude that timestamps for write and alter schema operations are monotonically increasing in unison with opid. The same cannot necessarily be said for timestamps of no-ops and change configs. This is a conservative conclusion about assigned timestamps, and this patch hinges on the fact that our Raft implementation ensures the following sequence of events: 1. replica A becomes leader of Term N 2. leader A assigns a timestamp t1 to its no-op 3. leader A replicates the no-op to replicas B and C, asserting its leadership for Term N 4. leader A prepares a write and assigns it a timestamp t2 > t1 5. leader A replicates the write to replicas B and C, checking that it is leader for the Term N Given the above series of operations, within the same term, the no-op used to assert leadership is always assigned a timestamp that must be lower than any other ops in that term. As such, the timestamps assigned to no-ops can and should be used to bump safetime. This patch does so at bootstrap time with committed no-ops found in the WAL. This alone isn't enough to prevent KUDU-2463, but it reduces the window during which incorrect results can be returned. Change-Id: I26deff32da8c990cb8a2ba220bb81858ddd6d73f --- M src/kudu/consensus/consensus.proto M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/raft_consensus-itest.cc A src/kudu/integration-tests/timestamp_advancement-itest.cc M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_bootstrap.cc 6 files changed, 276 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/11142/7 -- To view, visit http://gerrit.cloudera.org:8080/11142 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I26deff32da8c990cb8a2ba220bb81858ddd6d73f Gerrit-Change-Number: 11142 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-2463 pt 1: adjust MVCC when replaying no-ops
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11142 )
Change subject: KUDU-2463 pt 1: adjust MVCC when replaying no-ops
..
Patch Set 7:
(32 comments)
http://gerrit.cloudera.org:8080/#/c/11142/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11142/6//COMMIT_MSG@10
PS6, Line 10: r schema
> timestamps
Done
http://gerrit.cloudera.org:8080/#/c/11142/6//COMMIT_MSG@17
PS6, Line 17: cannot necessarily be said for timestamps of no-ops and change
configs.
> also schema changes (I think)
Good callout on schema changes, though I think they fall into the same bucket
as write ops. They both use the TransactionDriver and get their timestamps
assigned on the prepare thread.
http://gerrit.cloudera.org:8080/#/c/11142/6//COMMIT_MSG@37
PS6, Line 37: This alone isn't enough to prevent KUDU-2463, but it reduces the
window
> The cases where it doesn't prevent the problem are super edge case, right?
Right, with Part 2, this is a pretty narrow edge case.
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/consensus/consensus.proto
File src/kudu/consensus/consensus.proto:
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/consensus/consensus.proto@235
PS6, Line 235: If
> If
Done
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/consensus/consensus.proto@236
PS6, Line 236: to be true.
> to be true
Done
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/raft_consensus-itest.cc@259
PS6, Line 259: stamp
> is this needed for this test to pass? or just for debuggability upon test f
Yep, it's needed. Right now, the first no-op is assigned a timestamp of 1, so
we end up committing timestamp 1. With it, we update the clean time to 1. Then,
we try to replay WAL entries with timestamp 1 and crash because timestamp 1 has
already been committed.
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/timestamp_advancement-itest.cc
File src/kudu/integration-tests/timestamp_advancement-itest.cc:
PS6:
> Did clang-tidy run on this file?
Seems Adar found the issue with clang tidy
https://gerrit.cloudera.org/c/11457/
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/timestamp_advancement-itest.cc@78
PS6, Line 78: namespace itest {
> nit: hrm, is there a reason you put these outside the namespace? seems easi
Done
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/timestamp_advancement-itest.cc@79
PS6, Line 79:
> nit: out of alpha order
Done
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/timestamp_advancement-itest.cc@86
PS6, Line 86:
> not used?
Done
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/timestamp_advancement-itest.cc@90
PS6, Line 90: // Set a low batch size so we have finer-grained control over
flushing of
> not used?
Done
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/timestamp_advancement-itest.cc@94
PS6, Line 94: write.Start();
> not used
Done
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/timestamp_advancement-itest.cc@107
PS6, Line 107: // Flush the current log batch and roll over to get a fresh
WAL segment.
> nit: not used
Done
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/timestamp_advancement-itest.cc@108
PS6, Line 108: ASSERT_OK(tablet_replica->log()->WaitUntilAllFlushed());
> nit: out of alpha order
Done
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/timestamp_advancement-itest.cc@112
PS6, Line 112: SERT_OK(tablet_replica->ta
> nit: TimestampAdvancementITest ?
Done
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/timestamp_advancement-itest.cc@121
PS6, Line 121:
> this is the default
Done
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/timestamp_advancement-itest.cc@122
PS6, Line 122: // Get the tablet replica on the tablet server 'ts'.
: scoped_refptr tablet_replica_on_ts(int ts)
const {
: vector Why change this stuff? If no compelling reason, let's remove it so we don't
Done
http://gerrit.cloudera.org:8080/#/c/11142/6/src/kudu/integration-tests/timestamp_advancement-itest.cc@135
PS6, Line 135: RETURN_NOT_OK(LogReader::Open(env_,
:
ts->server()->fs_manager()->GetTabletWalDir(tablet_id),
: scoped_refptr(), tablet_id,
: scoped_refptr(), &reader));
: log::SegmentSequence segs;
: RETURN_NOT_OK(reader->GetSegmentsSnapshot(&segs));
: unique_ptr entry;
: for (const auto& seg : segs) {
: log::LogEntryReader reader(seg.ge
[kudu-CR] KUDU-2463 pt 1: adjust MVCC when replaying no-ops
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11142 ) Change subject: KUDU-2463 pt 1: adjust MVCC when replaying no-ops .. Patch Set 7: > Patch Set 7: > > (32 comments) Here's a dist test run: http://dist-test.cloudera.org/job?job_id=awong.1537249774.103150. Seems stable, 10-20 seconds with TSAN -- To view, visit http://gerrit.cloudera.org:8080/11142 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I26deff32da8c990cb8a2ba220bb81858ddd6d73f Gerrit-Change-Number: 11142 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 18 Sep 2018 15:14:05 + Gerrit-HasComments: No
[kudu-CR] KUDU-2580 [c++ client] authn token reacquisition fix
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11449 ) Change subject: KUDU-2580 [c++ client] authn token reacquisition fix .. Patch Set 1: (4 comments) If you want to backport it to older branches, I suggest making a more pristine patch with the various bits of cleanup removed. http://gerrit.cloudera.org:8080/#/c/11449/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11449/1//COMMIT_MSG@9 PS1, Line 9: Updated the authn token reacquisition logic to handle the following Could you add more detail here to make it more obvious why this leads to a failure? We talked about it over lunch yesterday and only at the very end of the discussion did I really understand what was happening. http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.h@165 PS1, Line 165: void ReconnectToCluster( Doc this. Maybe move the definition of ReconnectionReason down here too, since it's only relevant for this function. http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.h@166 PS1, Line 166: const MonoTime& deadline, : KuduClient* client, Nit: can you switch the argument order of these two, to reduce the delta between the new code and old code? http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.cc@158 PS1, Line 158: Status KuduClient::Data::SyncLeaderMasterRpc( So to repro the bug, which code branches down here are taken, in what order? -- To view, visit http://gerrit.cloudera.org:8080/11449 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4477d0f2bb36ee5ef580b585cae189a634d5002f Gerrit-Change-Number: 11449 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 18 Sep 2018 16:54:03 + Gerrit-HasComments: Yes
[kudu-CR] [tests] test scenario for KUDU-2580
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11448 )
Change subject: [tests] test scenario for KUDU-2580
..
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc
File src/kudu/integration-tests/authn_token_expire-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@106
PS2, Line 106: int num_tablet_servers,
: int num_masters)
Nit: if you're already changing the argument order, prefer num_masters before
num_tservers; that's how it's usually done in other tests, so this different
order is error prone.
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@431
PS2, Line 431:
Substitute("--leader_failure_max_missed_heartbeat_periods=$0",
: master_leader_failure_max_missed_heartbeat_periods_),
: Substitute("--raft_heartbeat_interval_ms=$0",
: master_raft_hb_interval_ms_),
: Substitute("--rpc_default_keepalive_time_ms=$0",
: master_rpc_keepalive_time_ms_),
Can you justify these customized settings?
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@437
PS2, Line 437: "-v=1",
Was this just for debugging? Remove it now?
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@497
PS2, Line 497: // Keep the connection to leader master open, time to time
making requests
: // that go to the leader master, but not to other masters in
the cluster.
Unless you disable failure detection in the masters, how can you guarantee that
a new leader master isn't elected for some random reason? Will that mess up the
test?
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@519
PS2, Line 519: SleepFor(MonoDelta::FromMilliseconds(3 *
master_raft_hb_interval_ms_));
Given that the whole thing is wrapped in ASSERT_EVENTUALLY, why do we need an
explicit sleep?
--
To view, visit http://gerrit.cloudera.org:8080/11448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58218deef24cca7c524bc61700cd400cdaabd050
Gerrit-Change-Number: 11448
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Tue, 18 Sep 2018 17:00:21 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2580 [c++ client] authn token reacquisition fix
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11449 ) Change subject: KUDU-2580 [c++ client] authn token reacquisition fix .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.cc@270 PS1, Line 270: if (resp->error().code() == MasterErrorPB::NOT_THE_LEADER || BTW, in our lunch conversation I recall you saying that one problem is that the client gets a NOT_THE_LEADER error instead of the properp authn failure. Is that something you're going to address separately? If not, why not? -- To view, visit http://gerrit.cloudera.org:8080/11449 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4477d0f2bb36ee5ef580b585cae189a634d5002f Gerrit-Change-Number: 11449 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 18 Sep 2018 17:02:07 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2437 Split a tablet into primary key ranges by size
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10406 ) Change subject: KUDU-2437 Split a tablet into primary key ranges by size .. Patch Set 23: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10406 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ec4395919f4b54102e458ef5154334c08412e8a Gerrit-Change-Number: 10406 Gerrit-PatchSet: 23 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yao Xu Gerrit-Comment-Date: Tue, 18 Sep 2018 17:02:58 + Gerrit-HasComments: No
[kudu-CR] KUDU-2437 Split a tablet into primary key ranges by size
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10406 ) Change subject: KUDU-2437 Split a tablet into primary key ranges by size .. KUDU-2437 Split a tablet into primary key ranges by size When reading data in a kudu table using spark, if there is a large amount of data in the tablet, reading the data takes a long time. The reason is that KuduRDD uses a tablet to generate the scanToken, so a spark task needs to process all the data in a tablet. TabletServer should provide an RPC interface, which can be split tablet into multiple primary key ranges by size. The kudu-client can choose whether to perform parallel scan according to the case. Change-Id: I9ec4395919f4b54102e458ef5154334c08412e8a Reviewed-on: http://gerrit.cloudera.org:8080/10406 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M src/kudu/common/CMakeLists.txt M src/kudu/common/common.proto A src/kudu/common/key_range.cc A src/kudu/common/key_range.h M src/kudu/tablet/cfile_set.cc M src/kudu/tablet/cfile_set.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h M src/kudu/tablet/rowset_info.cc M src/kudu/tablet/rowset_info.h M src/kudu/tablet/tablet-harness.h M src/kudu/tablet/tablet-test-util.h M src/kudu/tablet/tablet-test.cc M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h M src/kudu/tserver/tserver.proto M src/kudu/tserver/tserver_service.proto 23 files changed, 734 insertions(+), 3 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/10406 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I9ec4395919f4b54102e458ef5154334c08412e8a Gerrit-Change-Number: 10406 Gerrit-PatchSet: 24 Gerrit-Owner: Yao Xu Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Yao Xu
[kudu-CR] clang tidy gerrit.py: fix output when no changes found in first path
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11457 )
Change subject: clang_tidy_gerrit.py: fix output when no changes found in first
path
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11457/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11457/1//COMMIT_MSG@12
PS1, Line 12: what I saw in the console output of one such failed job run:
:
: Clang output
: No relevant changes found.
: No relevant changes found.
: ...
: Traceback (most recent call last):
: File "build-support/clang_tidy_gerrit.py", line 209, in
: parsed = parse_clang_output(clang_output)
: File "build-support/clang_tidy_gerrit.py", line 103, in
parse_clang_output
: raise Exception("bad w
> Without digging into the clang tidy script, I'm not sure why certain patche
"No relevant..." is what clang-tidy returns when it has nothing to recommend.
That's very unlikely when run on an entire patch (as we used to do), but after
commit a9271b05d, we actually run it once per changed file, so now it's
possible to see it as some files (especially new ones) are completely tidy.
To trigger the bug, you need the _very first_ file processed to produce this
message, at which point split_warnings() gets messed up and returns a warning
containing just the "No relevant..." line, which causes parse_clang_output() to
raise an exception, the script to abort, and no clang-tidy comments to
materialize in the gerrit.
--
To view, visit http://gerrit.cloudera.org:8080/11457
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I111dff7508f841489ba1625a4ca4b7af92f3d8d0
Gerrit-Change-Number: 11457
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 18 Sep 2018 17:18:20 +
Gerrit-HasComments: Yes
[kudu-CR] clang tidy gerrit.py: fix output when no changes found in first path
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11457
to look at the new patch set (#2).
Change subject: clang_tidy_gerrit.py: fix output when no changes found in first
path
..
clang_tidy_gerrit.py: fix output when no changes found in first path
For a while now some patches weren't getting any Tidy Bot comments. These
comments originate in a standalone Jenkins job that runs clang-tidy and
converts its output into comments that are posted back to gerrit. Here's
what I saw in the console output of one such failed job run:
Clang output
No relevant changes found.
No relevant changes found.
...
Traceback (most recent call last):
File "build-support/clang_tidy_gerrit.py", line 209, in
parsed = parse_clang_output(clang_output)
File "build-support/clang_tidy_gerrit.py", line 103, in parse_clang_output
raise Exception("bad warning: " + w)
Exception: bad warning: No relevant changes found.
No relevant changes found.
The "No relevant changes found." line is what clang-tidy prints when it has
no recommendations. That won't happen when clang-tidy's input includes at
least one "dirty" file, but as of commit a9271b05d we run clang-tidy in
parallel on a per-file basis, which makes it quite likely that a given patch
will include at least one completely tidy file.
Turns out that when the very first line of output parsed by this script is
that "No relevant..." line, split_warnings() generates a warning with a
non-warning string, which causes parse_clang_output() to raise an exception
and for the job to fail silently.
Change-Id: I111dff7508f841489ba1625a4ca4b7af92f3d8d0
---
M build-support/clang_tidy_gerrit.py
1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/11457/2
--
To view, visit http://gerrit.cloudera.org:8080/11457
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I111dff7508f841489ba1625a4ca4b7af92f3d8d0
Gerrit-Change-Number: 11457
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [hms] relax the notification format restriction
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11460 ) Change subject: [hms] relax the notification format restriction .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/11460/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11460/1//COMMIT_MSG@7 PS1, Line 7: [hms] relax the notification format restriction What motivated this change? http://gerrit.cloudera.org:8080/#/c/11460/1//COMMIT_MSG@10 PS1, Line 10: restrication restriction -- To view, visit http://gerrit.cloudera.org:8080/11460 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3 Gerrit-Change-Number: 11460 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 18 Sep 2018 17:40:24 + Gerrit-HasComments: Yes
[kudu-CR] [tests] test scenario for KUDU-2580
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11448 ) Change subject: [tests] test scenario for KUDU-2580 .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/11448/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11448/2//COMMIT_MSG@7 PS2, Line 7: tests nit: I would categorize tests with the subsystem they test, so [security] instead of [tests]. I think it's more useful for the log that way. http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc File src/kudu/integration-tests/authn_token_expire-itest.cc: http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@143 PS2, Line 143: 3 /* num_tablet_servers */ nit: The prevailing way to format this is /*num_tablet_servers=*/3 http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@144 PS2, Line 144: 1 /* num_masters */ Ditto. http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@360 PS2, Line 360: 2 /* token_validity_seconds */, : 3 /* num_tablet_servers */, : 1 /* num_masters * Ditto. http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@425 PS2, Line 425: 2 /* token_validity_seconds */, : 3 /* num_tablet_servers */, : 3 /* num_masters */ Ditto. -- To view, visit http://gerrit.cloudera.org:8080/11448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58218deef24cca7c524bc61700cd400cdaabd050 Gerrit-Change-Number: 11448 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 18 Sep 2018 18:20:15 + Gerrit-HasComments: Yes
[kudu-CR] clang tidy gerrit.py: fix output when no changes found in first path
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11457 ) Change subject: clang_tidy_gerrit.py: fix output when no changes found in first path .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11457 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I111dff7508f841489ba1625a4ca4b7af92f3d8d0 Gerrit-Change-Number: 11457 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 18 Sep 2018 18:19:56 + Gerrit-HasComments: No
[kudu-CR] clang tidy gerrit.py: fix output when no changes found in first path
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11457 )
Change subject: clang_tidy_gerrit.py: fix output when no changes found in first
path
..
Patch Set 2: Code-Review+2
(1 comment)
> Patch Set 2:
>
> (1 comment)
http://gerrit.cloudera.org:8080/#/c/11457/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11457/1//COMMIT_MSG@12
PS1, Line 12: what I saw in the console output of one such failed job run:
:
: Clang output
: No relevant changes found.
: No relevant changes found.
: ...
: Traceback (most recent call last):
: File "build-support/clang_tidy_gerrit.py", line 209, in
: parsed = parse_clang_output(clang_output)
: File "build-support/clang_tidy_gerrit.py", line 103, in
parse_clang_output
: raise Exception("bad w
> "No relevant..." is what clang-tidy returns when it has nothing to recommen
Got it. Thanks for the explanation (and the update)!
--
To view, visit http://gerrit.cloudera.org:8080/11457
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I111dff7508f841489ba1625a4ca4b7af92f3d8d0
Gerrit-Change-Number: 11457
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 18 Sep 2018 18:26:08 +
Gerrit-HasComments: Yes
[kudu-CR] [hms] relax the notification format restriction
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11460 ) Change subject: [hms] relax the notification format restriction .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11460/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11460/1//COMMIT_MSG@7 PS1, Line 7: [hms] relax the notification format restriction > What motivated this change? The motivation is to be compatible with Hive distributions that do not include 'messageFormat' is in NotificationEvent thrift object (more specifically, https://issues.apache.org/jira/browse/HIVE-10562). -- To view, visit http://gerrit.cloudera.org:8080/11460 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3 Gerrit-Change-Number: 11460 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 18 Sep 2018 18:37:07 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2463 pt 3: don't scan if MVCC hasn't moved
Hello Tidy Bot, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11428 to look at the new patch set (#3). Change subject: KUDU-2463 pt 3: don't scan if MVCC hasn't moved .. KUDU-2463 pt 3: don't scan if MVCC hasn't moved In cases when a tablet bootstrap yields an MvccManager whose safe time has not been advanced (e.g. if there are no write ops in the WAL), and the tablet has otherwise not bumped its MVCC timestamps (e.g. if it has not yet elected a leader), a scan (whose correctness depends on the MvccManager to determine what transactions have been applied) will return incorrect results. In the same way that we prevent compactions from occuring if MVCC's timestamps have not been moved, this patch prevents incorrect results from being returend from a scan by returning an error that can be retried elsewhere. Tests are added to attempt to scan in this state, verifying that we get an error. Change-Id: Idc0f77673e1f04a34ab1f5c1930bbaa2498b39bf --- M src/kudu/integration-tests/timestamp_advancement-itest.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.h M src/kudu/tablet/mvcc.cc M src/kudu/tablet/mvcc.h M src/kudu/tools/kudu-ts-cli-test.cc M src/kudu/tserver/tablet_service.cc 10 files changed, 206 insertions(+), 50 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/28/11428/3 -- To view, visit http://gerrit.cloudera.org:8080/11428 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idc0f77673e1f04a34ab1f5c1930bbaa2498b39bf Gerrit-Change-Number: 11428 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2463 pt 2: bump MVCC safe time on Raft no-op
Hello Tidy Bot, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11427 to look at the new patch set (#4). Change subject: KUDU-2463 pt 2: bump MVCC safe time on Raft no-op .. KUDU-2463 pt 2: bump MVCC safe time on Raft no-op Based on the same rationale as Part 1 of this patch series, this patch updates MVCC's safe and clean time using the no-op timestamp provided by the leader following a successful Raft election. There isn't an obvious reference to the tablet (to get to the MVCC module) in Raft consensus, but there is a ReplicaTransactionFactory, that the TabletReplica implements. I've extended this to be a more general ConsensusRoundHandler that can be used to create transactions or finish transactions as needed. One thing to note is that in some cases (e.g. brand new tablets), the first election would replicate the no-op with a timestamp of 1 (the timestamp we're trying to avoid). I tracked the cause of this to be the fact that sometimes the hybrid clock doesn't get updated before sending out the first no-op, and this will result in the first assigned timestamp being 1. To work around this, I updated the clock to the initial clean time, which seems in line with other updates to the hybrid clock in the time manager. I tested this in the following ways: - to ensure nothing terrible happens when there is a lot of election churn (and hence, a lot of timestamp advancement), I've tweaked exactly_once_writes-itest to actually churn elections. Previously it attempted this with just a low timeout; I injected some latency to make it churn a bit harder. - I added a test that ensured that, on its own, a tablet would bump its MVCC timestamps, by virtue of its elections - I tested the above with single-replica tablets as well - a few other tests needed to be tweaked given the extra bump to the hybrid clock This patch alone doesn't fix KUDU-2463. Rather, a later patch will prevent scans from occuring if the MVCC safe time hasn't been advanced, at which point this patch will reduce the window of scan unavailability. Change-Id: Icbf812e2cb7c322fd980245cfe40c886a15a --- M src/kudu/consensus/consensus-test-util.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/consensus/time_manager.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/exactly_once_writes-itest.cc M src/kudu/integration-tests/raft_consensus-itest.cc M src/kudu/integration-tests/timestamp_advancement-itest.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h 10 files changed, 166 insertions(+), 78 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/11427/4 -- To view, visit http://gerrit.cloudera.org:8080/11427 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icbf812e2cb7c322fd980245cfe40c886a15a Gerrit-Change-Number: 11427 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2463 pt 3: don't scan if MVCC hasn't moved
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11428 )
Change subject: KUDU-2463 pt 3: don't scan if MVCC hasn't moved
..
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/integration-tests/timestamp_advancement-itest.cc
File src/kudu/integration-tests/timestamp_advancement-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/integration-tests/timestamp_advancement-itest.cc@295
PS2, Line 295: ScanResponsePB resp;
: RpcController rpc;
: ScanRequestPB req = ReqForTablet(tablet_id);
: shared_ptr tserver_proxy =
cluster_->tserver_proxy(0);
: ASSERT_OK(tserver_proxy->Scan(req, &resp, &rpc));
: LOG(INFO) << "Got response: " << SecureShortDebugString(resp);
: ASSERT_FALSE(resp.has_error());
> Is it worth factoring this out into a helper method?
Done
http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/integration-tests/timestamp_advancement-itest.cc@354
PS2, Line 354: // Set a low Raft heartbeat.
: FLAGS_leader_failure_max_missed_heartbeat_periods = 10;
: FLAGS_raft_enable_pre_election = false;
: FLAGS_enable_maintenance_manager = false;
:
: // Lower the heartbeat interval so the follower watermarks
(that dictate when
: // we can GC logs) get updated quickly.
: FLAGS_raft_heartbeat_interval_ms = 10;
:
: scoped_refptr replica;
: NO_FATALS(SetupClusterWithWritesInWAL(0, &replica));
: MiniTabletServer* ts = tserver(0);
:
: const MonoDelta kTimeout = MonoDelta::FromSeconds(30);
: const string tablet_id = replica->tablet_id();
:
: // Update one of the followers repeatedly to generate a bunch
of config
: // changes in all the replicas' WALs.
: TServerDetails* leader;
: ASSERT_OK(FindTabletLeader(ts_map_, tablet_id, kTimeout,
&leader));
: vector followers;
: ASSERT_OK(FindTabletFollowers(ts_map_, tablet_id, kTimeout,
&followers));
: ASSERT_FALSE(followers.empty());
: for (int i = 0; i < 100; i++) {
: RaftPeerPB::MemberType type = i % 2 == 0 ?
RaftPeerPB::NON_VOTER : RaftPeerPB::VOTER;
: WARN_NOT_OK(ChangeReplicaType(leader, tablet_id,
followers[0], type, kTimeout),
: "Couldn't send a change config!");
: }
: LOG(INFO) << "Finished inserting to the WALs";
:
: ASSERT_EVENTUALLY([&] {
: int64_t gcable_size;
: ASSERT_OK(replica->GetGCableDataSize(&gcable_size));
: ASSERT_GT(gcable_size, 0);
: LOG(INFO) << "GCing logs...";
: ASSERT_OK(replica->RunLogGC());
: });
:
> nit: most of this should be factored out into a helper method, since it's c
Done
http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tablet/mvcc.h@190
PS2, Line 190: server
> serve
Done
http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tablet/mvcc.h@191
PS2, Line 191: CheckHasAdvancedTimestamps
> how about: CheckIsSafeTimeInitialized() ?
Done
http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tablet/mvcc.cc
File src/kudu/tablet/mvcc.cc:
http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tablet/mvcc.cc@52
PS2, Line 52: Aborted
> How about Uninitialized or maybe even ServiceUnavailable?
Done
http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tablet/mvcc.cc@52
PS2, Line 52: clean time
> I'm not sure we want to expose the concept of clean time to our users. Mayb
Done
http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/11428/2/src/kudu/tserver/tablet_service.cc@1767
PS2, Line 1767: *error_code = TabletServerErrorPB::TABLET_FAILED;
> Can we do something like service unavailable instead? According to the prot
Hrm, yeah I agree TABLET_FAILED is inappropriate. The closest thing we have to
unavailable is the rpc status ERROR_UNAVAILABLE, but that doesn't seem to fit
perfectly either, and I'm hesitant to add a new tserver error code.
How about TABLET_NOT_RUNNING? That's what we would use if the tablet were
bootstrapping, and it isn't too far of a stretch to think of this state of
waiting for consensus as an extension of bootstrapping.
--
To view, visit http://gerrit.cloudera.org:8080/11428
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-I
[kudu-CR] [hms] relax the notification format restriction
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11460 )
Change subject: [hms] relax the notification format restriction
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11460/1/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:
http://gerrit.cloudera.org:8080/#/c/11460/1/src/kudu/master/hms_notification_log_listener.cc@184
PS1, Line 184: if (!event.messageFormat.empty() && event.messageFormat !=
"json-0.2") {
Could you add a comment explaining what `messageFormat.empty()` means in terms
of parsing a message? Also, could you add some small tests that expect failures
when it's a non-empty non-"json-0.2" string?
--
To view, visit http://gerrit.cloudera.org:8080/11460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3
Gerrit-Change-Number: 11460
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 18 Sep 2018 18:48:40 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2463 pt 2: bump MVCC safe time on Raft no-op
Andrew Wong has removed a vote on this change. Change subject: KUDU-2463 pt 2: bump MVCC safe time on Raft no-op .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11427 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Icbf812e2cb7c322fd980245cfe40c886a15a Gerrit-Change-Number: 11427 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2463 pt 2: bump MVCC safe time on Raft no-op
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11427 ) Change subject: KUDU-2463 pt 2: bump MVCC safe time on Raft no-op .. Patch Set 4: Verified+1 The test failure was KUDU-2557, long-running TserverGoesDownDuringRebalancingTest.TserverDown/1 -- To view, visit http://gerrit.cloudera.org:8080/11427 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icbf812e2cb7c322fd980245cfe40c886a15a Gerrit-Change-Number: 11427 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 18 Sep 2018 19:23:52 + Gerrit-HasComments: No
[kudu-CR] [hms] relax the notification format restriction
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/11460 )
Change subject: [hms] relax the notification format restriction
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11460/1/src/kudu/master/hms_notification_log_listener.cc
File src/kudu/master/hms_notification_log_listener.cc:
http://gerrit.cloudera.org:8080/#/c/11460/1/src/kudu/master/hms_notification_log_listener.cc@184
PS1, Line 184: if (!event.messageFormat.empty() && event.messageFormat !=
"json-0.2") {
> Could you add a comment explaining what `messageFormat.empty()` means in te
Sure, I will add a comment. But I do not see much value of adding the test,
since it is straightforward that if "it's a non-empty non-json-0.2", then
NotSupported error with message "unknown message format" will be thrown.
--
To view, visit http://gerrit.cloudera.org:8080/11460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3
Gerrit-Change-Number: 11460
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 18 Sep 2018 19:26:23 +
Gerrit-HasComments: Yes
[kudu-CR] [hms] relax the notification format restriction
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11460 ) Change subject: [hms] relax the notification format restriction .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11460/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11460/1//COMMIT_MSG@10 PS1, Line 10: restriction > restriction Done -- To view, visit http://gerrit.cloudera.org:8080/11460 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3 Gerrit-Change-Number: 11460 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 18 Sep 2018 19:47:07 + Gerrit-HasComments: Yes
[kudu-CR] [hms] relax the notification format restriction
Hello Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11460 to look at the new patch set (#2). Change subject: [hms] relax the notification format restriction .. [hms] relax the notification format restriction 'messageFormat' is an optional field in NotificationEvent thrift object. This commit relaxes the restriction of HMS notification message format, so that the notification log listener can process the message even when the format type is not provided. Removing this restriction should not cause any regressions as long as the required fields are present in the messages. Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3 --- M src/kudu/master/hms_notification_log_listener.cc 1 file changed, 5 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/11460/2 -- To view, visit http://gerrit.cloudera.org:8080/11460 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3 Gerrit-Change-Number: 11460 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] clang tidy gerrit.py: fix output when no changes found in first path
Adar Dembo has submitted this change and it was merged. (
http://gerrit.cloudera.org:8080/11457 )
Change subject: clang_tidy_gerrit.py: fix output when no changes found in first
path
..
clang_tidy_gerrit.py: fix output when no changes found in first path
For a while now some patches weren't getting any Tidy Bot comments. These
comments originate in a standalone Jenkins job that runs clang-tidy and
converts its output into comments that are posted back to gerrit. Here's
what I saw in the console output of one such failed job run:
Clang output
No relevant changes found.
No relevant changes found.
...
Traceback (most recent call last):
File "build-support/clang_tidy_gerrit.py", line 209, in
parsed = parse_clang_output(clang_output)
File "build-support/clang_tidy_gerrit.py", line 103, in parse_clang_output
raise Exception("bad warning: " + w)
Exception: bad warning: No relevant changes found.
No relevant changes found.
The "No relevant changes found." line is what clang-tidy prints when it has
no recommendations. That won't happen when clang-tidy's input includes at
least one "dirty" file, but as of commit a9271b05d we run clang-tidy in
parallel on a per-file basis, which makes it quite likely that a given patch
will include at least one completely tidy file.
Turns out that when the very first line of output parsed by this script is
that "No relevant..." line, split_warnings() generates a warning with a
non-warning string, which causes parse_clang_output() to raise an exception
and for the job to fail silently.
Change-Id: I111dff7508f841489ba1625a4ca4b7af92f3d8d0
Reviewed-on: http://gerrit.cloudera.org:8080/11457
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin
Reviewed-by: Andrew Wong
---
M build-support/clang_tidy_gerrit.py
1 file changed, 5 insertions(+), 1 deletion(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/11457
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I111dff7508f841489ba1625a4ca4b7af92f3d8d0
Gerrit-Change-Number: 11457
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [hms] relax the notification format restriction
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11460 ) Change subject: [hms] relax the notification format restriction .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11460/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11460/1//COMMIT_MSG@7 PS1, Line 7: [hms] relax the notification format restriction > The motivation is to be compatible with Hive distributions that do not incl Thanks. Could you note that motivation explicitly in the commit message? -- To view, visit http://gerrit.cloudera.org:8080/11460 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3 Gerrit-Change-Number: 11460 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 18 Sep 2018 20:21:03 + Gerrit-HasComments: Yes
[kudu-CR] [hms] relax the notification format restriction
Hello Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11460 to look at the new patch set (#3). Change subject: [hms] relax the notification format restriction .. [hms] relax the notification format restriction 'messageFormat' is an optional field in NotificationEvent thrift object introduced in HIVE-10562. In order to be compatible with Hive distributions that do not include HIVE-10652, this commit relaxes the restriction of HMS notification message format, so that the notification log listener can process the message even when the format type is not provided. Removing this restriction should not cause any regressions as long as the required fields are present in the messages. Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3 --- M src/kudu/master/hms_notification_log_listener.cc 1 file changed, 5 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/11460/3 -- To view, visit http://gerrit.cloudera.org:8080/11460 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3 Gerrit-Change-Number: 11460 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [hms] relax the notification format restriction
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11460 ) Change subject: [hms] relax the notification format restriction .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11460 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3 Gerrit-Change-Number: 11460 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 18 Sep 2018 20:35:19 + Gerrit-HasComments: No
[kudu-CR] [hms] relax the notification format restriction
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11460 ) Change subject: [hms] relax the notification format restriction .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/11460/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11460/1//COMMIT_MSG@7 PS1, Line 7: [hms] relax the notification format restriction > Thanks. Could you note that motivation explicitly in the commit message? Done -- To view, visit http://gerrit.cloudera.org:8080/11460 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3 Gerrit-Change-Number: 11460 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 18 Sep 2018 20:36:07 + Gerrit-HasComments: Yes
[kudu-CR](gh-pages) Blogpost describing index skip scan optimization.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11263 ) Change subject: Blogpost describing index skip scan optimization. .. Patch Set 8: (8 comments) Sorry for the delay in reviewing this. This looks good, I just have a few additional nitpick points of feedback and then I think this is ready to post. Since we are getting ready to post this, we should change the date of the blog post to a date in the future. How about 2018-09-25 (next Tuesday), when we can post this? http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md File _posts/2018-08-17-index-skip-scan-optimization-in-kudu.md: http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@10 PS8, Line 10: nit: I would add something along these lines here to help transition to the next paragraph: I wanted to share my experience and the progress we've made so far on the approach. http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@36 PS8, Line 36: contains nit: "only contains the" http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@74 PS8, Line 74: http://latex.codecogs.com/gif.download?%5Csqrt%20%7B%20%5C%23rows%5C%20in%5C%20tablet%20%7D download this, check it in, and include it in the gerrit review please http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@76 PS8, Line 76: decide s/decide/have tentatively chosen/ http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@77 PS8, Line 77: http://latex.codecogs.com/gif.download?%5Csqrt%20%7B%20%5C%23rows%5C%20in%5C%20tablet%20%7D please check this in http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@78 PS8, Line 78: take s/take/project/ http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@88 PS8, Line 88: current implementation s/current implementation/implementation in the patch/ http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@109 PS8, Line 109: [[2]](https://oracle-base.com/articles/9i/index-skip-scanning/): Index Skip Scanning - Oracle Database : : [[3]](https://www.sqlite.org/optoverview.html#skipscan): Skip Scan - SQLite I'm not seeing references to these links in this blog post -- To view, visit http://gerrit.cloudera.org:8080/11263 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I2250652dcba3d1b0a06f1ffb7f23c11bf533d35e Gerrit-Change-Number: 11263 Gerrit-PatchSet: 8 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Anupama Gupta Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 18 Sep 2018 22:28:58 + Gerrit-HasComments: Yes
[kudu-CR](gh-pages) Blogpost describing index skip scan optimization.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11263 ) Change subject: Blogpost describing index skip scan optimization. .. Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md File _posts/2018-08-17-index-skip-scan-optimization-in-kudu.md: http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@74 PS8, Line 74: http://latex.codecogs.com/gif.download?%5Csqrt%20%7B%20%5C%23rows%5C%20in%5C%20tablet%20%7D > download this, check it in, and include it in the gerrit review please As an alternative approach, consider simple text representation sqrt(number_of_rows_in_tablet) Would fit as well? http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@77 PS8, Line 77: http://latex.codecogs.com/gif.download?%5Csqrt%20%7B%20%5C%23rows%5C%20in%5C%20tablet%20%7D > please check this in maybe, simple text representation would fit as well: sqrt(number_of_rows_in_tablet) ? -- To view, visit http://gerrit.cloudera.org:8080/11263 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I2250652dcba3d1b0a06f1ffb7f23c11bf533d35e Gerrit-Change-Number: 11263 Gerrit-PatchSet: 8 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Anupama Gupta Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 18 Sep 2018 22:58:04 + Gerrit-HasComments: Yes
[kudu-CR](gh-pages) Blogpost describing index skip scan optimization.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11263 ) Change subject: Blogpost describing index skip scan optimization. .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md File _posts/2018-08-17-index-skip-scan-optimization-in-kudu.md: http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@109 PS8, Line 109: [[2]](https://oracle-base.com/articles/9i/index-skip-scanning/): Index Skip Scanning - Oracle Database : : [[3]](https://www.sqlite.org/optoverview.html#skipscan): Skip Scan - SQLite > I'm not seeing references to these links in this blog post I see it now; feel free to ignore this. -- To view, visit http://gerrit.cloudera.org:8080/11263 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I2250652dcba3d1b0a06f1ffb7f23c11bf533d35e Gerrit-Change-Number: 11263 Gerrit-PatchSet: 8 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Anupama Gupta Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 18 Sep 2018 23:34:27 + Gerrit-HasComments: Yes
[kudu-CR](gh-pages) Blogpost describing index skip scan optimization.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11263 ) Change subject: Blogpost describing index skip scan optimization. .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md File _posts/2018-08-17-index-skip-scan-optimization-in-kudu.md: http://gerrit.cloudera.org:8080/#/c/11263/8/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@74 PS8, Line 74: http://latex.codecogs.com/gif.download?%5Csqrt%20%7B%20%5C%23rows%5C%20in%5C%20tablet%20%7D > As an alternative approach, consider simple text representation that would work too, yeah -- To view, visit http://gerrit.cloudera.org:8080/11263 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I2250652dcba3d1b0a06f1ffb7f23c11bf533d35e Gerrit-Change-Number: 11263 Gerrit-PatchSet: 8 Gerrit-Owner: Anupama Gupta Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Anupama Gupta Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 18 Sep 2018 23:34:49 + Gerrit-HasComments: Yes
[kudu-CR] [security] test scenario for KUDU-2580
Hello Will Berkeley, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11448 to look at the new patch set (#3). Change subject: [security] test scenario for KUDU-2580 .. [security] test scenario for KUDU-2580 Added a repro test scenario for KUDU-2580. The test is disabled since it's failing. The test will be re-enabled in a follow-up patch that fixes KUDU-2580. Change-Id: I58218deef24cca7c524bc61700cd400cdaabd050 --- M src/kudu/integration-tests/authn_token_expire-itest.cc 1 file changed, 178 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/11448/3 -- To view, visit http://gerrit.cloudera.org:8080/11448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I58218deef24cca7c524bc61700cd400cdaabd050 Gerrit-Change-Number: 11448 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2580 [c++ client] authn token reacquisition fix
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11449 ) Change subject: KUDU-2580 [c++ client] authn token reacquisition fix .. Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/11449/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11449/1//COMMIT_MSG@9 PS1, Line 9: Updated the authn token reacquisition logic to handle the following > Could you add more detail here to make it more obvious why this leads to a Done http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.h@165 PS1, Line 165: void ReconnectToCluster( > Doc this. Maybe move the definition of ReconnectionReason down here too, si Done http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.h@166 PS1, Line 166: const MonoTime& deadline, : KuduClient* client, > Nit: can you switch the argument order of these two, to reduce the delta be Done http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.cc@158 PS1, Line 158: Status KuduClient::Data::SyncLeaderMasterRpc( > So to repro the bug, which code branches down here are taken, in what order Having the connection to the former leader master still open, but authn token expired and master re-election selecting another master as leader, the GetTableSchema() RPC call hits the code of the last 'if ()' closure within the retry 'for ()' cycle, where the error code is MasterErrorPB::NOT_THE_LEADER. The ConnectToMaster() attempt would yield FATAL_INVALID_AUTHENTICATION_TOKEN error code, and the control flow will continue with the retries, but the state of the client stays the same (i.e. the leader master proxy hasn't been updated). That will happen again and again, until the RPC call of GetTableSchema() times out. As I mentioned in the prior changelist, I think it's worth refactoring the ConnectionToCluster code a bit and clean the cached master proxy in that case. But I think it's better to do that in a separate changelist, keeping this changelist targeted for back-porting to older maintenance branches. http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.cc@270 PS1, Line 270: if (resp->error().code() == MasterErrorPB::NOT_THE_LEADER || > BTW, in our lunch conversation I recall you saying that one problem is that The original approach of authn token re-acquisition is to go get reacquire authn token when the returned error code of the RPC call is FATAL_INVALID_AUTHENTICATION_TOKEN. That's straightforward and simple enough. However, that works only in case if authn token is present, i.e. that works only when establishing a new connection. Once connection is established, no authn token is sent to the server anymore. And even if it were sent, that might be some performance hit to verify authn token signature and check for expiration time at every RPC call. So, the former master leader simply cannot notify the client of its expired token since the master does not see the authn token during regular RPC calls. http://gerrit.cloudera.org:8080/#/c/11449/1/src/kudu/client/client-internal.cc@659 PS1, Line 659: s.IsNotAuthorized > I think here we might need to narrow down the case when we need to retry: p Will and I discussed this offline and it seems having this as-is for the purpose of back-porting this to older branches is OK. I'll send a follow-up patch for review with refactoring this code. -- To view, visit http://gerrit.cloudera.org:8080/11449 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4477d0f2bb36ee5ef580b585cae189a634d5002f Gerrit-Change-Number: 11449 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 19 Sep 2018 01:23:18 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2580 [c++ client] authn token reacquisition fix
Hello Will Berkeley, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11449 to look at the new patch set (#2). Change subject: KUDU-2580 [c++ client] authn token reacquisition fix .. KUDU-2580 [c++ client] authn token reacquisition fix This patch updates the authn token reacquisition logic to handle the following scenario: 1. Client is running against a multi-master cluster. 2. Client successfully authenticates and gets an authn token by calling ConnectToCluster(). 3. Client keeps the connection to leader master open, but follower masters close connections to the client due to inactivity. 4. After the authn token expires, a change in the master leadership happens. 5. Client tries to open a table, making a request to the former leader master. The former leader returns NOT_THE_LEADER error. The original authn token reacquisition logic was straightforwardly retrying RPCs in case of error responses with code FATAL_INVALID_AUTHENTICATION_TOKEN only. The scenario described above was not handled properly, so the opening table operation would fail with the following error: Timed out: GetTableSchema timed out after deadline expired Under the hood, the following would happen prior to this patch in the above scenario when calling the GetTableSchema(): * Having the connection to the former leader master still open, but authn token expired and master re-election selected another master as leader, the GetTableSchema() RPC call would hit the code of the last 'if ()' closure within the 'for ()' retry cycle in the KuduClient::Data::SyncLeaderMasterRpc() method, with error code MasterErrorPB::NOT_THE_LEADER. * The ConnectToMaster() attempt would return an error with the FATAL_INVALID_AUTHENTICATION_TOKEN error code, and the control flow will continue with the retries, but the state of the client stays the same (i.e. the leader master proxy is not updated). * That will happen again and again, until the GetTableSchema() RPC call times out. This patch also enables the ClientReacquiresAuthnToken scenario of MultiMasterIdleConnectionsITest since it passes with this fix. Change-Id: I4477d0f2bb36ee5ef580b585cae189a634d5002f --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/master_rpc.cc M src/kudu/integration-tests/authn_token_expire-itest.cc 4 files changed, 64 insertions(+), 33 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/11449/2 -- To view, visit http://gerrit.cloudera.org:8080/11449 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4477d0f2bb36ee5ef580b585cae189a634d5002f Gerrit-Change-Number: 11449 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [security] test scenario for KUDU-2580
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11448 )
Change subject: [security] test scenario for KUDU-2580
..
Patch Set 3:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/11448/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11448/2//COMMIT_MSG@7
PS2, Line 7: secur
> nit: I would categorize tests with the subsystem they test, so [security] i
Done
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc
File src/kudu/integration-tests/authn_token_expire-itest.cc:
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@106
PS2, Line 106: int num_tablet_servers)
: : token_validity_seconds_(token_validi
> Nit: if you're already changing the argument order, prefer num_masters befo
Done
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@143
PS2, Line 143: /*num_tablet_servers=*/ 3)
> nit: The prevailing way to format this is
Done
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@144
PS2, Line 144: FATAL_INVALID_AUTH
> Ditto.
Done
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@360
PS2, Line 360: /*num_masters=*/ 1,
: /*num_tablet_servers=*/ 3) {
: cluster_opts_.extra_mast
> Ditto.
Done
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@425
PS2, Line 425: /*num_masters=*/ 3,
: /*num_tablet_servers=*/ 3) {
:
> Ditto.
Done
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@431
PS2, Line 431: // expiration of authn tokens, while the default authn
expiration timeout
: // is 7 days. So, let's make the token validity interval
really short.
: Substitute("--authn_token_validity_seconds=$0",
token_validity_seconds_),
:
: // The default for
leader_failure_max_missed_heartbeat_periods 3.0, but
: // 2.0 is enough to have measter le
> Can you justify these customized settings?
Done
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@437
PS2, Line 437: // run a bit faster.
> Was this just for debugging? Remove it now?
Right, that's just some remnants from the debugging phase.
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@497
PS2, Line 497:
: shared_ptr client;
> Unless you disable failure detection in the masters, how can you guarantee
It's a good observation.
With a small change it should be fine, and I'll update this correspondingly.
The idea is to have a code where master re-election might mess up the test only
to the point that this scenario will not give repro for KUDU-2580 and would not
fail without the follow-up patch, but otherwise it should be safe -- it does
not bring flakiness in terms of intermittent failures when the fix for
KUDU-2580 is committed.
http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@519
PS2, Line 519: // but that would not induce errors in this cycle. The only
negative outcome
> Given that the whole thing is wrapped in ASSERT_EVENTUALLY, why do we need
Good catch -- indeed, that is not needed. That's a stale piece from the
initial approach.
--
To view, visit http://gerrit.cloudera.org:8080/11448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58218deef24cca7c524bc61700cd400cdaabd050
Gerrit-Change-Number: 11448
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Wed, 19 Sep 2018 01:23:41 +
Gerrit-HasComments: Yes
[kudu-CR] [hms] relax the notification format restriction
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11460 ) Change subject: [hms] relax the notification format restriction .. [hms] relax the notification format restriction 'messageFormat' is an optional field in NotificationEvent thrift object introduced in HIVE-10562. In order to be compatible with Hive distributions that do not include HIVE-10652, this commit relaxes the restriction of HMS notification message format, so that the notification log listener can process the message even when the format type is not provided. Removing this restriction should not cause any regressions as long as the required fields are present in the messages. Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3 Reviewed-on: http://gerrit.cloudera.org:8080/11460 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/master/hms_notification_log_listener.cc 1 file changed, 5 insertions(+), 1 deletion(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/11460 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I09d0e36cebe2074d975504d6bbc5677107e33fe3 Gerrit-Change-Number: 11460 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [hms] enable special characters support in mini HMS
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11467 Change subject: [hms] enable special characters support in mini HMS .. [hms] enable special characters support in mini HMS This commit explictly enables special characters support for mini HMS which allows '/' in table names, to avoid test failure (e.g. MasterHmsTest.TestUppercaseIdentifiers) caused by running with Hive distributions that disable special characters support by default. Change-Id: I3ae5387c6c39a3d0c622316e5cda3236660806a4 --- M src/kudu/hms/mini_hms.cc 1 file changed, 5 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/11467/1 -- To view, visit http://gerrit.cloudera.org:8080/11467 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I3ae5387c6c39a3d0c622316e5cda3236660806a4 Gerrit-Change-Number: 11467 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao
