[kudu-CR] KUDU-1625: background op to GC ancient, fully deleted rowsets
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/15145 )
Change subject: KUDU-1625: background op to GC ancient, fully deleted rowsets
..
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15145/10/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/15145/10/src/kudu/tablet/tablet.cc@1481
PS10, Line 1481: if (metadata_->supports_live_row_count()) {
> Hi, @Andrew
Keep in mind that Kudu uses the "ancient history mark" to refer to a point
before which snapshot scans will not work. But we still want to keep ancient
data around so scans later than the ancient history mark will still see what
the latest version of the data is, even if it was last updated several weeks
ago.
The goal of the DeletedRowsetGCOp is to free up storage for fully deleted
rowsets that are also entirely ancient. Such rowsets aren't useful since any
scan after the ancient history mark will always apply all the delete redos and
return no rows. It's thus safe to entirely ignore the diskrowset entirely and
get rid of them.
We can't make the same case for data that hasn't been fully deleted, because
the base data may still contain data that is still scannable.
IIRC, without the live row count, we'd need to open each delta store to
determine whether a diskrowset is fully deleted. Without doing that, I'm not
sure there's a safe way to enable this without live row count support.
--
To view, visit http://gerrit.cloudera.org:8080/15145
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I696e2a29ea52ad4e54801b495c322bc371787124
Gerrit-Change-Number: 15145
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Lieber-Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Volodymyr Verovkin
Gerrit-Reviewer: Yingchun Lai
Gerrit-Comment-Date: Thu, 10 Nov 2022 19:04:22 +
Gerrit-HasComments: Yes
[kudu-CR] [master] Add a rebalance request/response and provide a tool, refact master's rebalance
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/18402 )
Change subject: [master] Add a rebalance request/response and provide a tool,
refact master's rebalance
..
Patch Set 14:
(3 comments)
Not a full review, just thought I'd put in the request to break this up
http://gerrit.cloudera.org:8080/#/c/18402/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18402/14//COMMIT_MSG@8
PS14, Line 8:
: 1. Refact master's auto rebalance, enable/disable auto rebalancer
: need not restart master.
:
: 2. Add a rebalance request/response, user can trigger rebalance
task,
: even if the rebalancer is disabled.
:
: 3. Add a new rebalance tool, and compatible with incoming's leader
: rebalance.
:
Thanks for all these changes! But please split these into individual patches
with unit tests for each logical behavior. It will be much easier for reviewers
to review that way.
http://gerrit.cloudera.org:8080/#/c/18402/14/src/kudu/master/auto_rebalancer.cc
File src/kudu/master/auto_rebalancer.cc:
http://gerrit.cloudera.org:8080/#/c/18402/14/src/kudu/master/auto_rebalancer.cc@277
PS14, Line 277: #define NEXT_EXECUTE_FUNCTION
\
This is never called, it seems.
http://gerrit.cloudera.org:8080/#/c/18402/14/src/kudu/master/auto_rebalancer.cc@300
PS14, Line 300: if (moves_scheduled_this_round_for_test_ == 0) {
: // To Avoid double leaders.
: SleepFor(MonoDelta::FromMilliseconds(5 *
FLAGS_raft_heartbeat_interval_ms));
: }
What was the behavioral change that led to this? I expected there to be few
functional changes in this patch.
--
To view, visit http://gerrit.cloudera.org:8080/18402
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d506d877662c029a30dca9dd5029b9da4162946
Gerrit-Change-Number: 18402
Gerrit-PatchSet: 14
Gerrit-Owner: Yuqi Du
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du
Gerrit-Comment-Date: Sat, 07 May 2022 07:09:53 +
Gerrit-HasComments: Yes
[kudu-CR] [tools] update schema if needed when rebuild master
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18496 ) Change subject: [tools] update schema if needed when rebuild master .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/18496/3/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/18496/3/src/kudu/tools/kudu-admin-test.cc@3245 PS3, Line 3245: ScanTableToStrings(table.get(), ); nit: could we explicitly also check that the table has the right number of columns? http://gerrit.cloudera.org:8080/#/c/18496/3/src/kudu/tools/master_rebuilder.cc File src/kudu/tools/master_rebuilder.cc: http://gerrit.cloudera.org:8080/#/c/18496/3/src/kudu/tools/master_rebuilder.cc@230 PS3, Line 230: table->mutable_metadata()->CommitMutation(); nit: maybe just put this after L246? http://gerrit.cloudera.org:8080/#/c/18496/3/src/kudu/tools/master_rebuilder.cc@253 PS3, Line 253: nit: maybe log a message saying something like "ignoring mismatched schema since it has a lower version" or somesuch? -- To view, visit http://gerrit.cloudera.org:8080/18496 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec99d57115228b521ba645b8e19c7057a4bb5d3d Gerrit-Change-Number: 18496 Gerrit-PatchSet: 3 Gerrit-Owner: Yingchun Lai Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Sat, 07 May 2022 07:07:00 + Gerrit-HasComments: Yes
[kudu-CR] [threadpool] KUDU-3364 Add TimerThread for thread pool
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/18447 )
Change subject: [threadpool] KUDU-3364 Add TimerThread for thread pool
..
Patch Set 13:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/18447/13//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18447/13//COMMIT_MSG@12
PS13, Line 12: trigge
nit: trigger
http://gerrit.cloudera.org:8080/#/c/18447/13//COMMIT_MSG@14
PS13, Line 14: refact
nit: refactor
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@126
PS13, Line 126: enable_timer
nit: how about "enable_scheduling" or somesuch
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@127
PS13, Line 127: precise_ms
nit: precise_ms isn't particularly descriptive. How about
"set_scheduler_period_ms" or something
Same for L143, L153, and L180
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@150
PS13, Line 150: periodic
nit: maybe I'm missing something -- I don't see any periodic scheduling
happening in this patch.
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@151
PS13, Line 151: TimerThread
nit: "timer" makes it seem like this should be measuring the time of tasks. How
about instead calling this SchedulerThread or somesuch?
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@156
PS13, Line 156:
: Status Start();
:
: Status Shutdown();
nit: please do your best to describe the behaviors of these methods in comments
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@168
PS13, Line 168: timepoint
nit: I think "start_time" would be more descriptive
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@178
PS13, Line 178: std::string thread_pool_name_;
: // timer's period checking time.
: uint32_t precise_ms_;
These can be const
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.h@238
PS13, Line 238: NORMAL,
: TEST
nit: these names are not very descriptive. How about something like
enum class ScheduledTaskWaitType {
WAIT,
NO_WAIT,
};
Though frankly, I'm not sure it's worth adding this enum in general just for
the sake of saving a second in one unit test.
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc@142
PS13, Line 142: MutexLock auto_lock(mutex_);
: while (true) {
: const auto it = tasks_.begin();
: if (it != tasks_.end() && it->first <= now) {
: ThreadPoolToken* token = it->second.thread_pool_token_;
: CHECK_OK(token->Submit(it->second.f));
: tasks_.erase(it);
: task_size_--;
: } else {
: break;
: }
: }
: }
nit: I think it'd be a bit more understandable as
MutexLock l(mutex_);
while (!tasks_.empty()) {
auto it = tasks_.begin();
if (it->first > now) {
break;
}
ThreadPoolToken* token = it->second.thread_pool_token_;
CHECK_OK(token->Submit(it->second.f));
tasks_.erase(it);
task_size_--;
}
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc@246
PS13, Line 246: if (mode() != ThreadPool::ExecutionMode::SERIAL) {
: return Status::NotSupported(Substitute("unsupported mode:
CONCURRENT"));
: }
nit: this isn't behavior we ever expect to reach in production -- it is a bug
to write code that calls Schedule() when not set to SERIAL. So, let's use a
CHECK or DCHECK instead.
http://gerrit.cloudera.org:8080/#/c/18447/13/src/kudu/util/threadpool.cc@641
PS13, Line 641: timer_->task_size_ > 0
nit: if this is the only usage of task_size_, maybe get rid of it and instead
just use !tasks_.empty()
--
To view, visit http://gerrit.cloudera.org:8080/18447
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If51fce48ae6a45a0bf3314f8a102ee373c5c442e
Gerrit-Change-Number: 18447
Gerrit-PatchSet: 13
Gerrit-Owner: Yuqi Du
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Khazar Mammadli
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yuqi Du
Gerrit-Comment-Date: Sat, 07 May 2022 06:50:58 +
Gerrit-HasComments: Yes
[kudu-CR] [java] Fix a scan bug which will read repetitive rows.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18420 ) Change subject: [java] Fix a scan bug which will read repetitive rows. .. Patch Set 28: Thanks for addressing all the feedback! -- To view, visit http://gerrit.cloudera.org:8080/18420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d6be9df10c1a45cd971b52a0351028c1f5a023f Gerrit-Change-Number: 18420 Gerrit-PatchSet: 28 Gerrit-Owner: Yuqi Du Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Comment-Date: Sat, 07 May 2022 05:05:22 + Gerrit-HasComments: No
[kudu-CR] [java] Fix a scan bug which will read repetitive rows.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18420 ) Change subject: [java] Fix a scan bug which will read repetitive rows. .. [java] Fix a scan bug which will read repetitive rows. When isFaultTolerant is true, from 2nd ScanRequest, in its response callback function, lastPrimaryKey is not updated. In common scenarios, when tablet server hosting the leader replica restarts, Scanners will read rows from the first ScanResponse's lastPrimaryKey, that will return some repetitive rows. Change-Id: I4d6be9df10c1a45cd971b52a0351028c1f5a023f Reviewed-on: http://gerrit.cloudera.org:8080/18420 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/RowErrorsAndOverflowStatus.java M java/kudu-client/src/test/java/org/apache/kudu/client/ITFaultTolerantScanner.java M java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java 4 files changed, 160 insertions(+), 3 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/18420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I4d6be9df10c1a45cd971b52a0351028c1f5a023f Gerrit-Change-Number: 18420 Gerrit-PatchSet: 28 Gerrit-Owner: Yuqi Du Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du
[kudu-CR] [java] Fix a scan bug which will read repetitive rows.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18420 ) Change subject: [java] Fix a scan bug which will read repetitive rows. .. Patch Set 27: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d6be9df10c1a45cd971b52a0351028c1f5a023f Gerrit-Change-Number: 18420 Gerrit-PatchSet: 27 Gerrit-Owner: Yuqi Du Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Comment-Date: Sat, 07 May 2022 05:04:05 + Gerrit-HasComments: No
[kudu-CR] KUDU-1644: Simplify InList predicate values based on rowset PK bounds
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18434 ) Change subject: KUDU-1644: Simplify InList predicate values based on rowset PK bounds .. Patch Set 2: Code-Review+1 (2 comments) Would be great if you could rebase; I'm not sure what test failures there were here. http://gerrit.cloudera.org:8080/#/c/18434/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18434/2//COMMIT_MSG@11 PS2, Line 11: Also, the DRS bounds info can be used to skip rows : effectively when we have a predicate on a non-prefix of the primary key and the : leading column(s) have cardinality=1 (as described in KUDU-1291). > Yes, we can use the bound information to seek where we start to scan in a r I see, so it's more that for rowsets with leading column cardinality = 1, the the per-rowset ScanSpec optimization reduces to a skip scan. Sounds good. Doesn't this optimization also extend beyond InList predicates? Since it's using lower and uppser bounds, wouldn't it apply to range predicates as well? http://gerrit.cloudera.org:8080/#/c/18434/2/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/18434/2/src/kudu/tablet/cfile_set.cc@439 PS2, Line 439: spec->SetLowerBoundKey(implicit_lb_key); > Because we create a ScanSpec copy for each CFileSet Iterator: https://githu Ah, makes sense. Thanks! -- To view, visit http://gerrit.cloudera.org:8080/18434 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia9c2aa958f19a0b62e40a2ef5eb5365f91cbab80 Gerrit-Change-Number: 18434 Gerrit-PatchSet: 2 Gerrit-Owner: Yifan Zhang Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang Gerrit-Comment-Date: Sat, 07 May 2022 01:19:39 + Gerrit-HasComments: Yes
[kudu-CR] [tools] use thread pool for 'kudu local replica copy from local'
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18448 ) Change subject: [tools] use thread pool for 'kudu local_replica copy_from_local' .. Patch Set 8: Code-Review+2 Thanks for the improvement! -- To view, visit http://gerrit.cloudera.org:8080/18448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4de49e948cc1a686db5e1bf424470ca9e800ee36 Gerrit-Change-Number: 18448 Gerrit-PatchSet: 8 Gerrit-Owner: Yingchun Lai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Sat, 07 May 2022 01:10:54 + Gerrit-HasComments: No
[kudu-CR] [java] Fix a scan bug which will read repetitive rows.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18420 ) Change subject: [java] Fix a scan bug which will read repetitive rows. .. Patch Set 26: Code-Review+1 (1 comment) Basically LGTM, but would like a comment added http://gerrit.cloudera.org:8080/#/c/18420/24/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java File java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java: http://gerrit.cloudera.org:8080/#/c/18420/24/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@204 PS24, Line 204: this.token = token; > Simply, I need scan request at least 2 times. That makes sense. It is a good practice to add details like this in comments. Please add a comment so it's clear to maintainers and future code authors why this is important for this test. -- To view, visit http://gerrit.cloudera.org:8080/18420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d6be9df10c1a45cd971b52a0351028c1f5a023f Gerrit-Change-Number: 18420 Gerrit-PatchSet: 26 Gerrit-Owner: Yuqi Du Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Comment-Date: Sat, 07 May 2022 01:08:15 + Gerrit-HasComments: Yes
[kudu-CR] [java] Fix a scan bug which will read repetitive rows.
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/18420 )
Change subject: [java] Fix a scan bug which will read repetitive rows.
..
Patch Set 24:
(5 comments)
Sorry for the delay! Looking better!
http://gerrit.cloudera.org:8080/#/c/18420/24/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java
File
java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java:
http://gerrit.cloudera.org:8080/#/c/18420/24/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@83
PS24, Line 83: assertTrue(isDone ||
harness.getClient().isCreateTableDone(TABLE_NAME));
Why was this necessary? Doesn't the createTable() call wait for the table
creation to complete by default? Or would it make sense to increase the timeout?
I'm also curious why this change is necessary in the first place. It doesn't
seem like this patch affects table creation.
http://gerrit.cloudera.org:8080/#/c/18420/24/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@94
PS24, Line 94: id != Integer.MIN_VALUE &&
What's special about MIN_VALUE here?
http://gerrit.cloudera.org:8080/#/c/18420/24/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@93
PS24, Line 93: int id = random.nextInt();
: if (id != Integer.MIN_VALUE && !primaryKeys.contains(id)) {
: row.addInt(0, id);
: primaryKeys.add(id);
: } else {
: i--;
: continue;
: }
nit: I think it'd be a bit more straightforward to read this code as:
int key = random.nextInt();
while (primaryKeys.contains(key)) {
key = random.nextInt();
}
row.addInt(0, key);
primaryKeys.add(key);
http://gerrit.cloudera.org:8080/#/c/18420/24/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@204
PS24, Line 204: .batchSizeBytes(1)
nit: please comment _why_ it's important for this test to have a small batch
size. What client-side behavior are we trying to encourage?
http://gerrit.cloudera.org:8080/#/c/18420/24/java/kudu-client/src/test/java/org/apache/kudu/client/ITScannerMultiTablet.java@279
PS24, Line 279: rowCount += result.get();
Should we check for -1 errors here too?
--
To view, visit http://gerrit.cloudera.org:8080/18420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d6be9df10c1a45cd971b52a0351028c1f5a023f
Gerrit-Change-Number: 18420
Gerrit-PatchSet: 24
Gerrit-Owner: Yuqi Du
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang
Gerrit-Reviewer: Yingchun Lai
Gerrit-Reviewer: Yuqi Du
Gerrit-Comment-Date: Wed, 04 May 2022 05:31:09 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1644: Simplify InList predicate values based on rowset PK bounds
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18434 ) Change subject: KUDU-1644: Simplify InList predicate values based on rowset PK bounds .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/18434/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18434/2//COMMIT_MSG@11 PS2, Line 11: Also, the DRS bounds info can be used to skip rows : effectively when we have a predicate on a non-prefix of the primary key and the : leading column(s) have cardinality=1 (as described in KUDU-1291). Was this implemented in this patch? I'm not sure I saw it. http://gerrit.cloudera.org:8080/#/c/18434/2/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/18434/2/src/kudu/tablet/cfile_set.cc@439 PS2, Line 439: spec->SetLowerBoundKey(implicit_lb_key); I'm confused about why we're able to modify the ScanSpec for every CFileSet. The ScanSpec is shared for a single tablet scan, isn't it? Won't modifying it for one CFileSet interfere with the iteration of another CFileSet? -- To view, visit http://gerrit.cloudera.org:8080/18434 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia9c2aa958f19a0b62e40a2ef5eb5365f91cbab80 Gerrit-Change-Number: 18434 Gerrit-PatchSet: 2 Gerrit-Owner: Yifan Zhang Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 03 May 2022 19:07:46 + Gerrit-HasComments: Yes
[kudu-CR] wip KUDU-3357
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18478 Change subject: wip KUDU-3357 .. wip KUDU-3357 - RPC advertised addresses are now exposed through a different channel than the 'rpc_addresses' of the ServerRegistrationPB. WIP: - don't add a new field - instead, reuse rpc_addresses, and add a flag that dictates whether to resolve addresses - advertised_addresses should be sent to clients - need to distinguish when to use the advertised addresses - it's not necessarily the case that we want to use internal addresses for tserver traffic - maybe add flag where tserver is allowed to not expose internal address? Change-Id: I19025048dbc9bada3704c04140b131b7b15c5b96 --- M src/kudu/client/client.cc M src/kudu/client/meta_cache.cc M src/kudu/common/wire_protocol.proto M src/kudu/integration-tests/dns_alias-itest.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/ts_descriptor.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/server/rpc_server.cc M src/kudu/server/rpc_server.h M src/kudu/server/server_base.cc M src/kudu/tserver/heartbeater.cc 12 files changed, 183 insertions(+), 32 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/18478/1 -- To view, visit http://gerrit.cloudera.org:8080/18478 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I19025048dbc9bada3704c04140b131b7b15c5b96 Gerrit-Change-Number: 18478 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] util: pull jwt code from Impala
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18467 Change subject: util: pull jwt code from Impala .. util: pull jwt code from Impala Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b --- M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt-util-internal.h A src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt-util.cc A src/kudu/util/jwt-util.h M src/kudu/util/promise.h 6 files changed, 2,541 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/18467/1 -- To view, visit http://gerrit.cloudera.org:8080/18467 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] jwt: Java client usage of JWTs
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18477 Change subject: jwt: Java client usage of JWTs .. jwt: Java client usage of JWTs WIP: - this very barely plumbs JWTs into the Java-side negotiation - it doesn't correctly authenticate using plumbed JWTs, as seen by the test failure when only supplying a JWT and requiring authentication: org.apache.kudu.test.TestMiniKuduCluster > testJwt FAILED org.apache.kudu.client.NonRecoverableException: Couldn't find a valid master in (127.94.224.62:42399). Exceptions received: [org.apache.kudu.client.NonRecoverableException: client requires authentication, but server does not have Kerberos enabled] The end goal of this patch series is to have a Nifi client fetch a JWT from an endpoint, and use that to authenticate with a Kudu cluster that is unable to use Kerberos. Change-Id: Id02dcd7ee57a838f4763500e78053e21aac21c09 --- M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M java/kudu-client/src/main/java/org/apache/kudu/client/SecurityContext.java M java/kudu-test-utils/src/test/java/org/apache/kudu/test/TestMiniKuduCluster.java 3 files changed, 52 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/18477/1 -- To view, visit http://gerrit.cloudera.org:8080/18477 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Id02dcd7ee57a838f4763500e78053e21aac21c09 Gerrit-Change-Number: 18477 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] server: link in kudu jwt util
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18470 Change subject: server: link in kudu_jwt_util .. server: link in kudu_jwt_util Change-Id: Icfe694d553ebead6afbf58dc773bf5534f1d099a --- M src/kudu/server/CMakeLists.txt M src/kudu/server/server_base.cc 2 files changed, 4 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/18470/1 -- To view, visit http://gerrit.cloudera.org:8080/18470 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Icfe694d553ebead6afbf58dc773bf5534f1d099a Gerrit-Change-Number: 18470 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] jwt: introduce MiniOidc
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18473 Change subject: jwt: introduce MiniOidc .. jwt: introduce MiniOidc This patch takes some existing test utilities and encapsulates them into the new MiniOidc class. The MiniOidc serves the purpose of being the OpenID Connect Discovery endpoint, as the host for JWKSs of each account, and the dispensor of JWTs. This encapsulation will be useful in testing JWTs from non-C++ tests that typically reply on exposing Mini* components via control shell. Change-Id: I26e9b3bcd0946adbe4642a19c5ef1124e39632c6 --- M src/kudu/util/CMakeLists.txt M src/kudu/util/jwt-util-test.cc M src/kudu/util/jwt-util.cc M src/kudu/util/jwt-util.h A src/kudu/util/jwt.h A src/kudu/util/mini_oidc.cc A src/kudu/util/mini_oidc.h 7 files changed, 336 insertions(+), 136 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/18473/1 -- To view, visit http://gerrit.cloudera.org:8080/18473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I26e9b3bcd0946adbe4642a19c5ef1124e39632c6 Gerrit-Change-Number: 18473 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] jwt: plumb JWT into mini cluster
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18475 Change subject: jwt: plumb JWT into mini cluster .. jwt: plumb JWT into mini cluster This patch adds options to ExternalMiniCluster to start a MiniOidc alongside the calling process. Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653 --- M src/kudu/integration-tests/security-itest.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/util/CMakeLists.txt A src/kudu/util/jwt_test_certs.cc M src/kudu/util/jwt_test_certs.h 7 files changed, 450 insertions(+), 303 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/18475/1 -- To view, visit http://gerrit.cloudera.org:8080/18475 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Id0d3e53b60933ada0194afbe0ad4775be649b653 Gerrit-Change-Number: 18475 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] jwt: add test for fetching JWKS via URL
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18468 Change subject: jwt: add test for fetching JWKS via URL .. jwt: add test for fetching JWKS via URL Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8 --- M src/kudu/util/CMakeLists.txt M src/kudu/util/jwt-util-test.cc M src/kudu/util/jwt-util.cc 3 files changed, 75 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18468/1 -- To view, visit http://gerrit.cloudera.org:8080/18468 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8 Gerrit-Change-Number: 18468 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] jwt: determine discovery endpoint from token
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18472 Change subject: jwt: determine discovery endpoint from token .. jwt: determine discovery endpoint from token This patch introduces a JWT verifier that manages multiple JWKSs, depending on an account ID included in a given JWT's payload. The current implementation is somewhat crude, simply instantiating new JWTHelpers per account ID, thereby creating new threads per account. Follow-on work will be required to ensure scalability (though it's unclear how many account IDs to expect in a typical deployment). Change-Id: I970bcc6d894c0206160196418d549b71c35ac973 --- M src/kudu/rpc/negotiation.cc M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/server/server_base.cc M src/kudu/util/jwt-util-test.cc M src/kudu/util/jwt-util.cc M src/kudu/util/jwt-util.h 7 files changed, 300 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/18472/1 -- To view, visit http://gerrit.cloudera.org:8080/18472 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I970bcc6d894c0206160196418d549b71c35ac973 Gerrit-Change-Number: 18472 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] rpc: plumb JWTs into the RPC layer
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18469 Change subject: rpc: plumb JWTs into the RPC layer .. rpc: plumb JWTs into the RPC layer This patch plumbs JWT verification into our C++ RPC negotiation. It is limited in the sense that JWTs can be sent over unencrypted channels -- this should be addressed before using in production. Change-Id: I252f1e597d9df4408379c3b695f266dbd7f48dcc --- M src/kudu/client/client.proto M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/client_negotiation.h M src/kudu/rpc/messenger.cc M src/kudu/rpc/messenger.h M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/negotiation.cc M src/kudu/rpc/negotiation.h M src/kudu/rpc/rpc_header.proto M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/server_negotiation.h M src/kudu/security/token.proto M src/kudu/util/jwt-util.cc M src/kudu/util/jwt-util.h 14 files changed, 172 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/18469/1 -- To view, visit http://gerrit.cloudera.org:8080/18469 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I252f1e597d9df4408379c3b695f266dbd7f48dcc Gerrit-Change-Number: 18469 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] jwt: add control points for test binaries
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18476 Change subject: jwt: add control points for test binaries .. jwt: add control points for test binaries This hooks the configurations of the MiniOidc into the Java test binary, allowing test code to add statically-defined JWKS associated with specific account IDs, to be fetched via OIDC Discovery Endpoint. Change-Id: I489a67e93610467c5a2caabcf3e5603cbb49d118 --- M java/kudu-test-utils/src/main/java/org/apache/kudu/test/cluster/MiniKuduCluster.java M java/kudu-test-utils/src/test/java/org/apache/kudu/test/TestMiniKuduCluster.java M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_test.cc 4 files changed, 96 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/18476/1 -- To view, visit http://gerrit.cloudera.org:8080/18476 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I489a67e93610467c5a2caabcf3e5603cbb49d118 Gerrit-Change-Number: 18476 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] jwt: expose MiniOidc to Kudu test binary
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18474 Change subject: jwt: expose MiniOidc to Kudu test binary .. jwt: expose MiniOidc to Kudu test binary Change-Id: I397913fb5f1f2634b71b35f8c91f895b44e73be9 --- M src/kudu/tools/tool.proto 1 file changed, 35 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/18474/1 -- To view, visit http://gerrit.cloudera.org:8080/18474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I397913fb5f1f2634b71b35f8c91f895b44e73be9 Gerrit-Change-Number: 18474 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] plumb JWT authentication into clients
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18471 Change subject: plumb JWT authentication into clients .. plumb JWT authentication into clients Change-Id: Ibdfc2640c047a2e3bb5ea03aea4439cea2532e23 --- M src/kudu/client/client.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/rpc/client_negotiation.cc M src/kudu/rpc/server_negotiation.cc M src/kudu/security/tls_handshake.cc M src/kudu/server/server_base.cc M src/kudu/util/jwt-util-test.cc A src/kudu/util/jwt_test_certs.h 8 files changed, 453 insertions(+), 329 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/18471/1 -- To view, visit http://gerrit.cloudera.org:8080/18471 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ibdfc2640c047a2e3bb5ea03aea4439cea2532e23 Gerrit-Change-Number: 18471 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] [tools] use thread pool for 'kudu local replica copy from local'
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/18448 )
Change subject: [tools] use thread pool for 'kudu local_replica copy_from_local'
..
Patch Set 5:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/18448/5/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/18448/5/src/kudu/tools/tool_action_common.cc@136
PS5, Line 136: . Each thread runs its own "
: "KuduSession.
nit: since this is being used in common code, maybe remove this. Or consider
adding a separate flag for the copy tool
http://gerrit.cloudera.org:8080/#/c/18448/5/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
http://gerrit.cloudera.org:8080/#/c/18448/5/src/kudu/tools/tool_action_local_replica.cc@448
PS5, Line 448: vector
nit: just FYI this can also be a set, to deduplicate up front. That way some of
the sets we maintain below could just be vectors
http://gerrit.cloudera.org:8080/#/c/18448/5/src/kudu/tools/tool_action_local_replica.cc@483
PS5, Line 483: INFO
nit: maybe make this a warning or error?
http://gerrit.cloudera.org:8080/#/c/18448/5/src/kudu/tools/tool_action_local_replica.cc@483
PS5, Line 483: "Tablet $0 copy failed, error=Not found in source filesystem."
nit: maybe "Tablet $0 copy failed, not found in source filesystem"
http://gerrit.cloudera.org:8080/#/c/18448/5/src/kudu/tools/tool_action_local_replica.cc@498
PS5, Line 498: LOG(INFO) << copying_replica->last_status();
Just curious, does this also print the tablet IDs? If not, it might be worth
considering
http://gerrit.cloudera.org:8080/#/c/18448/5/src/kudu/tools/tool_action_local_replica.cc@536
PS5, Line 536: do {
: s = client.Start(tablet_id, /* meta */ nullptr);
: if (PREDICT_FALSE(!s.ok())) {
: s = s.CloneAndPrepend("Start copy tablet failed");
: break;
: }
: s = client.FetchAll(fake_replica);
: if (PREDICT_FALSE(!s.ok())) {
: s = s.CloneAndPrepend("Fetch all tablet data failed");
: break;
: }
: s = client.Finish();
: if (PREDICT_FALSE(!s.ok())) {
: s = s.CloneAndPrepend("Finish copy tablet failed");
: break;
: }
: } while (false);
nit: consider writing as
Status s = client.Start(tablet_id, nullptr).AndThen([&] {
return client.FetchAll(fake_replica);
}).AndThen([&] {
return client.Finish();
})
http://gerrit.cloudera.org:8080/#/c/18448/5/src/kudu/tools/tool_action_local_replica.cc@558
PS5, Line 558: Tablet $0 copy failed, error=$1.
nit: maybe "Tablet $0 copy failed: $1"
--
To view, visit http://gerrit.cloudera.org:8080/18448
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4de49e948cc1a686db5e1bf424470ca9e800ee36
Gerrit-Change-Number: 18448
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang
Gerrit-Reviewer: Yingchun Lai
Gerrit-Comment-Date: Sat, 30 Apr 2022 05:35:17 +
Gerrit-HasComments: Yes
[kudu-CR] [client] fix toggling fault-tolerance for a scanner
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18462 ) Change subject: [client] fix toggling fault-tolerance for a scanner .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/18462/1/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/18462/1/src/kudu/client/scan_token-test.cc@1085 PS1, Line 1085: ASSERT_EQ(KuduScanner::READ_AT_SNAPSHOT, sc.read_mode()); > It turns out the code in Java client assumes that switching to non-fault-to Right, I might expect that kind of behavior in some ResetToDefault() call or somesuch, but it seemed a bit odd otherwise. Things like scanner.SetReadMode(READ_AT_SNAPSHOT); scanner.SetFaultTolerant(false); would be have surprising results in that case IMO -- To view, visit http://gerrit.cloudera.org:8080/18462 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic50f9d62623d8b9ced7d854ce0e80a0faaa60ad3 Gerrit-Change-Number: 18462 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 30 Apr 2022 05:14:30 + Gerrit-HasComments: Yes
[kudu-CR] [client] fix toggling fault-tolerance for a scanner
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18462 ) Change subject: [client] fix toggling fault-tolerance for a scanner .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/18462/1/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/18462/1/src/kudu/client/scan_token-test.cc@1085 PS1, Line 1085: ASSERT_EQ(KuduScanner::ReadMode::READ_LATEST, sc.read_mode()); This seems a bit surprising -- why not have SetFaultTolerant(false) leave the read mode as is? -- To view, visit http://gerrit.cloudera.org:8080/18462 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic50f9d62623d8b9ced7d854ce0e80a0faaa60ad3 Gerrit-Change-Number: 18462 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 30 Apr 2022 01:13:38 + Gerrit-HasComments: Yes
[kudu-CR] [java] Fix a scan bug which will read repetitive rows.
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/18420 )
Change subject: [java] Fix a scan bug which will read repetitive rows.
..
Patch Set 15:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18420/15//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18420/15//COMMIT_MSG@12
PS15, Line 12: f
nit: often
http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
File
java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java:
http://gerrit.cloudera.org:8080/#/c/18420/15/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@693
PS15, Line 693: if (isFaultTolerant && resp.lastPrimaryKey != null) {
Nice catch! BTW, in looking at the differences between 'cb' defined at L558,
and this gotNextRow callback, I'm curious: should we be updating other fields?
Like the propagated timestamp? Or resource metrics?
It shouldn't be in this patch, but thought I'd raise the question, while we're
in this area of the code. Are there good reasons we _shouldn't_ be updating
those?
--
To view, visit http://gerrit.cloudera.org:8080/18420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d6be9df10c1a45cd971b52a0351028c1f5a023f
Gerrit-Change-Number: 18420
Gerrit-PatchSet: 15
Gerrit-Owner: Yuqi Du
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang
Gerrit-Reviewer: Yingchun Lai
Gerrit-Reviewer: Yuqi Du
Gerrit-Comment-Date: Fri, 22 Apr 2022 06:31:57 +
Gerrit-HasComments: Yes
[kudu-CR] [test] avoid coredump in socket-test when failing
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18425 ) Change subject: [test] avoid coredump in socket-test when failing .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18425 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5cf8f7b9acfac7b1935e6e6d9b3ac5016ee753d9 Gerrit-Change-Number: 18425 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yuqi Du Gerrit-Comment-Date: Tue, 19 Apr 2022 00:07:57 + Gerrit-HasComments: No
[kudu-CR](gh-pages) Add .asf.yaml
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18410 ) Change subject: Add .asf.yaml .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/18410/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18410/1//COMMIT_MSG@7 PS1, Line 7: Add .asf.yaml Mind adding some context about what the file actually does and why it's important? Does something in the release process break? -- To view, visit http://gerrit.cloudera.org:8080/18410 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: I58d8f85e83ac2ad46d192abb58b748eb7bba6c98 Gerrit-Change-Number: 18410 Gerrit-PatchSet: 1 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Andrew Wong Gerrit-Comment-Date: Thu, 14 Apr 2022 21:41:21 + Gerrit-HasComments: Yes
[kudu-CR] [tool] Add tool to copy replica from local filesystem
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18374 ) Change subject: [tool] Add tool to copy replica from local filesystem .. Patch Set 17: Thanks for the contribution! -- To view, visit http://gerrit.cloudera.org:8080/18374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dcafeaad900b66f297914760c54dba887874e95 Gerrit-Change-Number: 18374 Gerrit-PatchSet: 17 Gerrit-Owner: Yingchun Lai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Thu, 14 Apr 2022 21:39:02 + Gerrit-HasComments: No
[kudu-CR] [tool] Add tool to copy replica from local filesystem
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18374 ) Change subject: [tool] Add tool to copy replica from local filesystem .. [tool] Add tool to copy replica from local filesystem Add tool to copy replica from local filesystem, it would be faster than copy from remote peers, it can be use when add more disk drivers and make data rebalanced faster between disk drivers, or can be used when migrate data from the only disk driver to another. This tool will also make data more dense than data on old data directories, that means we can save much disk space and speedup server bootstrap. We can use the tool like: kudu local_replica copy_from_local [-src_fs_wal_dir=] [-src_fs_metadata_dir=] [-src_fs_data_dirs=] [-dst_fs_wal_dir=] [-dst_fs_metadata_dir=] [-dst_fs_data_dirs=] There are some tips to use this tool: - Using --src_* and --dst_* prefixes to clarify what directories are operating on - The server on the source filesystem must stop before coping replicas. Because the tool is standalone, it can't anchor replica's data blocks or log segments. - This tool will not delete data in the source filesystem, you should delete data manully if you want, by using kudu local_replica delete ... Change-Id: I1dcafeaad900b66f297914760c54dba887874e95 Reviewed-on: http://gerrit.cloudera.org:8080/18374 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/master/master_runner.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tserver/mini_tablet_server.cc M src/kudu/tserver/mini_tablet_server.h M src/kudu/tserver/tablet_copy-test-base.h M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_service.h M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/ts_tablet_manager.cc 19 files changed, 1,040 insertions(+), 243 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/18374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I1dcafeaad900b66f297914760c54dba887874e95 Gerrit-Change-Number: 18374 Gerrit-PatchSet: 17 Gerrit-Owner: Yingchun Lai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [tool] Add tool to copy replica from local filesystem
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18374 ) Change subject: [tool] Add tool to copy replica from local filesystem .. Patch Set 16: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dcafeaad900b66f297914760c54dba887874e95 Gerrit-Change-Number: 18374 Gerrit-PatchSet: 16 Gerrit-Owner: Yingchun Lai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Thu, 14 Apr 2022 21:38:45 + Gerrit-HasComments: No
[kudu-CR] [tool] Add tool to copy replica from local filesystem
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18374 ) Change subject: [tool] Add tool to copy replica from local filesystem .. Patch Set 10: (2 comments) http://gerrit.cloudera.org:8080/#/c/18374/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18374/7//COMMIT_MSG@21 PS7, Line 21: - Using --src_* and --dst_* prefixes to clarify what directories > > a user should stand up a new Kudu tserver at the same machine Thanks for clarifying. So with the first usage pattern in mind, doesn't destination file system have to have the same UUID? Is that a requirement? Otherwise, once the new tablet server is started, all existing Raft quorums will think the new server is not a part of the quorum. Regardless, could you add an end-to-end test for the usage pattern you described (copy a few local tablets, then start up the server, and make sure ksck is healthy) Agreed regarding online process -- definitely trickier to handle races with runtime operations. http://gerrit.cloudera.org:8080/#/c/18374/10/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/18374/10/src/kudu/tserver/tablet_copy_client.cc@1096 PS10, Line 1096: "" nit: why not set this to something like "local replica "? -- To view, visit http://gerrit.cloudera.org:8080/18374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dcafeaad900b66f297914760c54dba887874e95 Gerrit-Change-Number: 18374 Gerrit-PatchSet: 10 Gerrit-Owner: Yingchun Lai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Tue, 12 Apr 2022 18:30:59 + Gerrit-HasComments: Yes
[kudu-CR](gh-pages) Update website for 1.16.0 release
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18406 ) Change subject: Update website for 1.16.0 release .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18406 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: comment Gerrit-Change-Id: Iad600d36c0a739aa66ff4df5223668f17470a2dd Gerrit-Change-Number: 18406 Gerrit-PatchSet: 1 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Comment-Date: Tue, 12 Apr 2022 18:20:48 + Gerrit-HasComments: No
[kudu-CR] [tool] Add tool to copy replica from local filesystem
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/18374 )
Change subject: [tool] Add tool to copy replica from local filesystem
..
Patch Set 7:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/18374/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18374/7//COMMIT_MSG@21
PS7, Line 21: - Using --src_* and --dst_* prefixes to clarify what directories
So just checking my understanding, the idea here is that a user should stand up
a new Kudu tserver at the same machine, but mounting a different set of
directories. Once set up, the user runs this tool to copy tablets from one
local tserver to the other, and once complete, starts up the new tserver?
I also wonder if you considered an online process for rebalancing data (eg
through a maintenance manager thread, or a tweak in compaction prioritization
or somesuch).
http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tools/tool_action_local_replica.cc@482
PS7, Line 482: LOG(INFO) << fake_replica->last_status();
I'm not sure how much value this logging adds, over just doing it in the tool's
main thread. Presumably, fake_replica->last_status() will be OK for the
duration of the tool, right? And we seems like we do have some logging
throughout the client's life cycle. Alternatively, maybe consider instantiating
the tablet_copy_client_metrics and outputting them?
http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client-test.cc
File src/kudu/tserver/tablet_copy_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client-test.cc@20
PS7, Line 20:
nit: can drop the extra line
http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:
http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.h@197
PS7, Line 197: const boost::optional&
copy_peer_uuid = boost::none);
nit: why not rely on an empty string? Or pass in the local fs UUID?
It's not too important, but it is a bit awkward as is, since this can be valid,
invalid ("(unknown uuid)"), or none
http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.cc
File src/kudu/tserver/tablet_copy_client.cc:
http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.cc@352
PS7, Line 352: CHECK_OK
Why not RETURN_NOT_OK? Won't the tool exit early anyway?
http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.cc@1055
PS7, Line 1055: pb_util::SecureShortDebugString(session_->tablet_superblock()));
nit: the superblock might be huge, and it might be difficult to figure out what
the state actually is. Consider using `TabletDataState_Name(data_state)`?
http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_client.cc@1097
PS7, Line 1097: CHECK_OK
nit: RETURN_NOT_OK? I think this can fail if there's no space, so it'd be nice
to fail gracefully
http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_source_session.h
File src/kudu/tserver/tablet_copy_source_session.h:
http://gerrit.cloudera.org:8080/#/c/18374/7/src/kudu/tserver/tablet_copy_source_session.h@243
PS7, Line 243: std::string session_id_;
: std::string requestor_uuid_;
nit: these can both be const
--
To view, visit http://gerrit.cloudera.org:8080/18374
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcafeaad900b66f297914760c54dba887874e95
Gerrit-Change-Number: 18374
Gerrit-PatchSet: 7
Gerrit-Owner: Yingchun Lai
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai
Gerrit-Comment-Date: Mon, 11 Apr 2022 19:05:37 +
Gerrit-HasComments: Yes
[kudu-CR] [cmake modules] add '-o /dev/null' to the linker command
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18394 ) Change subject: [cmake_modules] add '-o /dev/null' to the linker command .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id095902679a2f0e2648c45bcd4024fc734b9a1e7 Gerrit-Change-Number: 18394 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 08 Apr 2022 04:24:42 + Gerrit-HasComments: No
[kudu-CR] [tools] range rebalancing for 'kudu cluster rebalance'
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18294 ) Change subject: [tools] range rebalancing for 'kudu cluster rebalance' .. Patch Set 7: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/18294/7/src/kudu/tools/rebalancer_tool.cc File src/kudu/tools/rebalancer_tool.cc: http://gerrit.cloudera.org:8080/#/c/18294/7/src/kudu/tools/rebalancer_tool.cc@614 PS7, Line 614: string prev_table_id; Given the limitation that the rebalancer in this mode can only run on a single table, does the prev_table_id actually get used, or is this more future proofing? -- To view, visit http://gerrit.cloudera.org:8080/18294 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7d2e19266e993f5e2ae13ba18d323c83db30eac1 Gerrit-Change-Number: 18294 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 04 Apr 2022 18:07:06 + Gerrit-HasComments: Yes
[kudu-CR] [tools] range rebalancing for 'kudu cluster rebalance'
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/18294 )
Change subject: [tools] range rebalancing for 'kudu cluster rebalance'
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18294/4/src/kudu/tools/rebalancer_tool.cc
File src/kudu/tools/rebalancer_tool.cc:
http://gerrit.cloudera.org:8080/#/c/18294/4/src/kudu/tools/rebalancer_tool.cc@602
PS4, Line 602: DataTable skew_table({ "UUID", "Server address",
"Replica Coun
> There is a table per range -- you can see how it looks in the description o
Right, but it only shows the details for a single table, IIUC.
I meant instead of only showing the "Per-range replica distribution details for
tables" table, maybe it's worth also a summary of skew per range, i.e. replica
count and replica skew, similar to the "Per-table replica distribution details"
table below.
I don't feel strongly about it, but was curious since it could be helpful to
get a big picture of which table ranges in the cluster need rebalancing.
--
To view, visit http://gerrit.cloudera.org:8080/18294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2e19266e993f5e2ae13ba18d323c83db30eac1
Gerrit-Change-Number: 18294
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 02 Apr 2022 04:54:06 +
Gerrit-HasComments: Yes
[kudu-CR] [tools] range rebalancing for 'kudu cluster rebalance'
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/18294 )
Change subject: [tools] range rebalancing for 'kudu cluster rebalance'
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18294/4/src/kudu/tools/rebalancer_tool.cc
File src/kudu/tools/rebalancer_tool.cc:
http://gerrit.cloudera.org:8080/#/c/18294/4/src/kudu/tools/rebalancer_tool.cc@602
PS4, Line 602: DataTable skew({ "UUID", "Server address", "Replica
Count" });
> I think the output here makes a lot of sense, especially to dig into the sp
Actually I realize now that my suggestion doesn't apply, since the limitation
exists to run on a single table.
That said, perhaps it's worth adding the ranges to the skew table below.
--
To view, visit http://gerrit.cloudera.org:8080/18294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2e19266e993f5e2ae13ba18d323c83db30eac1
Gerrit-Change-Number: 18294
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 02 Apr 2022 01:50:57 +
Gerrit-HasComments: Yes
[kudu-CR] [tools] range rebalancing for 'kudu cluster rebalance'
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/18294 )
Change subject: [tools] range rebalancing for 'kudu cluster rebalance'
..
Patch Set 4: Code-Review+1
(2 comments)
Overall this looks good to go! Thanks for the great contribution!
http://gerrit.cloudera.org:8080/#/c/18294/4/src/kudu/rebalance/rebalance_algo-test.cc
File src/kudu/rebalance/rebalance_algo-test.cc:
http://gerrit.cloudera.org:8080/#/c/18294/4/src/kudu/rebalance/rebalance_algo-test.cc@998
PS4, Line 998: TEST(RebalanceAlgoUnitTest, FewMovesSameTableRanges) {
nit: consider adding some cases for when there's a mix of both having some
tablets with ranges and others without
http://gerrit.cloudera.org:8080/#/c/18294/4/src/kudu/tools/rebalancer_tool.cc
File src/kudu/tools/rebalancer_tool.cc:
http://gerrit.cloudera.org:8080/#/c/18294/4/src/kudu/tools/rebalancer_tool.cc@602
PS4, Line 602: DataTable skew({ "UUID", "Server address", "Replica
Count" });
I think the output here makes a lot of sense, especially to dig into the
specifics of each range.
Curious if you think it's worth adding a similar skew table that summarizes the
results? I think it could be helpful as a summary, though worry showing both by
default might be too verbose.
--
To view, visit http://gerrit.cloudera.org:8080/18294
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7d2e19266e993f5e2ae13ba18d323c83db30eac1
Gerrit-Change-Number: 18294
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sat, 02 Apr 2022 01:47:13 +
Gerrit-HasComments: Yes
[kudu-CR] [tools] add beginning of the key range into TabletSummary
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18356 ) Change subject: [tools] add beginning of the key range into TabletSummary .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18356 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf083e1ae33919200f6e0b0f0a667ee6eea1bab3 Gerrit-Change-Number: 18356 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 01 Apr 2022 04:07:36 + Gerrit-HasComments: No
[kudu-CR] [tools] add beginning of the key range into TabletSummary
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18356 ) Change subject: [tools] add beginning of the key range into TabletSummary .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/18356/1/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: PS1: Might be worth updating some tests in ksck_remote-test.cc with some checks for tablets with real ranges, and tablets without any range specified. -- To view, visit http://gerrit.cloudera.org:8080/18356 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf083e1ae33919200f6e0b0f0a667ee6eea1bab3 Gerrit-Change-Number: 18356 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 28 Mar 2022 23:48:47 + Gerrit-HasComments: Yes
[kudu-CR] [docs] add notes on running chronyd in local reference mode
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18344 ) Change subject: [docs] add notes on running chronyd in local reference mode .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18344 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie346ed02f1faa52bce1c919cf89584d2cb398c7b Gerrit-Change-Number: 18344 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Mar 2022 17:51:56 + Gerrit-HasComments: No
[kudu-CR] [security] set minimum TLS protocol version to TSLv1.2
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17268 ) Change subject: [security] set minimum TLS protocol version to TSLv1.2 .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17268 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I07633a04d3828100f148e5de3905094198d13396 Gerrit-Change-Number: 17268 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Kurt Deschler Gerrit-Comment-Date: Wed, 23 Mar 2022 22:43:40 + Gerrit-HasComments: No
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 ) Change subject: KUDU-3318 [LBM] Runtime compact log container metadata .. Patch Set 12: Thanks for the contribution! -- To view, visit http://gerrit.cloudera.org:8080/17871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6 Gerrit-Change-Number: 17871 Gerrit-PatchSet: 12 Gerrit-Owner: Yingchun Lai Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Wed, 23 Mar 2022 05:54:08 + Gerrit-HasComments: No
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 ) Change subject: KUDU-3318 [LBM] Runtime compact log container metadata .. Patch Set 12: Verified+1 Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6 Gerrit-Change-Number: 17871 Gerrit-PatchSet: 12 Gerrit-Owner: Yingchun Lai Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Wed, 23 Mar 2022 05:53:50 + Gerrit-HasComments: No
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Andrew Wong has removed a vote on this change. Change subject: KUDU-3318 [LBM] Runtime compact log container metadata .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/17871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6 Gerrit-Change-Number: 17871 Gerrit-PatchSet: 12 Gerrit-Owner: Yingchun Lai Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [docs] add notes on running chronyd in local reference mode
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18344 ) Change subject: [docs] add notes on running chronyd in local reference mode .. Patch Set 1: Code-Review+1 (5 comments) http://gerrit.cloudera.org:8080/#/c/18344/1/docs/troubleshooting.adoc File docs/troubleshooting.adoc: http://gerrit.cloudera.org:8080/#/c/18344/1/docs/troubleshooting.adoc@518 PS1, Line 518: = Notes on running chronyd in local reference mode nit: surround in backticks http://gerrit.cloudera.org:8080/#/c/18344/1/docs/troubleshooting.adoc@519 PS1, Line 519: to make clock : of all nodes synchronised to one another nit: maybe "to synchronize the clocks on all nodes with one another" http://gerrit.cloudera.org:8080/#/c/18344/1/docs/troubleshooting.adoc@529 PS1, Line 529: chronyd nit: backticks http://gerrit.cloudera.org:8080/#/c/18344/1/docs/troubleshooting.adoc@530 PS1, Line 530: 3.4 version Is this this versions and below? Above? Or just, at least this version and possibly more (just based on testing) http://gerrit.cloudera.org:8080/#/c/18344/1/docs/troubleshooting.adoc@530 PS1, Line 530: chronyd nit: backticks -- To view, visit http://gerrit.cloudera.org:8080/18344 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie346ed02f1faa52bce1c919cf89584d2cb398c7b Gerrit-Change-Number: 18344 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Mar 2022 05:49:50 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3318 [LBM] Runtime compact log container metadata
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 ) Change subject: KUDU-3318 [LBM] Runtime compact log container metadata .. Patch Set 11: Code-Review+2 (4 comments) Thanks for revving this! Just more nits, otherwise LGTM. http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc@89 PS11, Line 89: DEFINE_uint64(log_container_metadata_max_size, 0, : "Maximum size (soft) of a log container's metadata. Use 0 for " : "no limit."); Empirically, and for posterity, I'm curious what values have been useful in resolving the space issues you've seen in your clusters. I imagine this would be directly correlated with the --log_container_max_size configuration, since the size of each metadata record is fairly constant, though with some wiggle room for deletes. http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc@123 PS11, Line 123: at startup nit: "..., or at runtime, based on the configuration of --log_container_metadata_size_before_compact_ratio." http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc@127 PS11, Line 127: "Desired ratio of block metadata size of --log_container_metadata_max_size. " : "If a container's metadata file size exceed --log_container_metadata_max_size * " : "--log_container_metadata_size_before_compact_ratio, the container is going " : "to be compact at runtime." nit: IMO it isn't clear from this what the behavior is, especially in considering log_container_live_metadata_before_compact_ratio. How about something like, "Desired portion of --log_container_metadata_max_size container metadata must consume before metadata is considered for compaction. If a container's metadata file exceeds --log_container_metadata_max_size * --log_container_metadata_size_before_compact_ratio, and the number of live blocks falls below --log_container_live_metadata_before_compact_ratio, the container's metadata will be compacted." http://gerrit.cloudera.org:8080/#/c/17871/11/src/kudu/fs/log_block_manager.cc@868 PS11, Line 868: // Check again is necessary, metadata may has been compacted very before. nit: maybe "Check again, in case the metadata was compacted while we were waiting." -- To view, visit http://gerrit.cloudera.org:8080/17871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6 Gerrit-Change-Number: 17871 Gerrit-PatchSet: 11 Gerrit-Owner: Yingchun Lai Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Mon, 21 Mar 2022 22:21:54 + Gerrit-HasComments: Yes
[kudu-CR](branch-1.16.x) Bump versions in examples to 1.16.0
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18335 ) Change subject: Bump versions in examples to 1.16.0 .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18335 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.16.x Gerrit-MessageType: comment Gerrit-Change-Id: I7ce1d9923dffc4e6ef311457b1c726162370b8b5 Gerrit-Change-Number: 18335 Gerrit-PatchSet: 1 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 21 Mar 2022 17:36:42 + Gerrit-HasComments: No
[kudu-CR] [tools] KUDU-3333 Include Table Counts in kudu hms Dryrun
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18280 ) Change subject: [tools] KUDU- Include Table Counts in kudu hms Dryrun .. [tools] KUDU- Include Table Counts in kudu hms Dryrun In cases where the user running the Kudu CLI tool, kudu hms fix, doesn't have permissions from Ranger/Sentry to access the tables, these tables would be treated as non-existant tables in Kudu. In such scenarios, there might be situations where the tables could be dropped from HMS inspite of them being present in Kudu when run with -drop_orphan_tables flag. This patch adds additional logging which reports the total table counts from HMS and Kudu master catalogs and warns the user if there are no tables in Kudu when kudu hms fix command is run. Sample runs of the tool before and after the change: In case of an empty cluster no output is seen without the code change. After the code change we see the below: $ ./kudu hms fix `hostname -f` I0315 16:16:36.039008 351197 tool_action_hms.cc:867] Number of Kudu tables found in Kudu master catalog: 0 I0315 16:16:36.039080 351197 tool_action_hms.cc:868] Number of Kudu tables found in HMS catalog: 0 $ ./kudu hms fix --dryrun `hostname -f` I0315 16:16:55.158463 351291 tool_action_hms.cc:642] NOTE: There are zero kudu tables listed. If the cluster indeed has kudu tables please re-run the command with right credentials. I0315 16:16:55.158546 351291 tool_action_hms.cc:867] Number of Kudu tables found in Kudu master catalog: 0 I0315 16:16:55.158555 351291 tool_action_hms.cc:868] Number of Kudu tables found in HMS catalog: 0 In case of a non-empty cluster without the change: $ kudu hms fix --dryrun `hostname -f` --ignore_other_clusters=false I0315 16:57:55.329049 365038 tool_action_hms.cc:757] [dryrun] Refreshing HMS table metadata for Kudu table default.my_first_table [id=408e5696e51c462c86a6d9a84bb95583] Non-empty cluster after the change: $ ./kudu hms fix --dryrun `hostname -f` I0315 16:19:20.885208 352393 tool_action_hms.cc:822] [dryrun] Changing owner of default.my_first_table [id=408e5696e51c462c86a6d9a84bb95583] to admin in Kudu catalog. I0315 16:19:20.885274 352393 tool_action_hms.cc:853] [dryrun] Refreshing HMS table metadata for Kudu table default.my_first_table [id=408e5696e51c462c86a6d9a84bb95583] I0315 16:19:20.885285 352393 tool_action_hms.cc:867] Number of Kudu tables found in Kudu master catalog: 1 I0315 16:19:20.885325 352393 tool_action_hms.cc:868] Number of Kudu tables found in HMS catalog: 1 Change-Id: Idf26141d2a3fd6cbb7249b3492fc6a50a0c0aa2d Reviewed-on: http://gerrit.cloudera.org:8080/18280 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M src/kudu/tools/tool_action_hms.cc 1 file changed, 21 insertions(+), 4 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/18280 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Idf26141d2a3fd6cbb7249b3492fc6a50a0c0aa2d Gerrit-Change-Number: 18280 Gerrit-PatchSet: 4 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [write throttling-itest] relax threshold to avoid flakiness
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18320 ) Change subject: [write_throttling-itest] relax threshold to avoid flakiness .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18320 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iad3335595b02e66cdc588755b8f53c77442d5736 Gerrit-Change-Number: 18320 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 17 Mar 2022 20:27:09 + Gerrit-HasComments: No
[kudu-CR] [tools] run intra-location rebalancing in parallel
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18308 ) Change subject: [tools] run intra-location rebalancing in parallel .. Patch Set 2: Code-Review+2 LGTM. For posterity, it's also worth noting that this was tested on a large production cluster and it did significantly speed up their rebalancing. -- To view, visit http://gerrit.cloudera.org:8080/18308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie4fe3ef3ec2fcac57114c97d5b6cd81d5d9953c4 Gerrit-Change-Number: 18308 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 17 Mar 2022 20:26:14 + Gerrit-HasComments: No
[kudu-CR] [tools] KUDU-3333 Include Table Counts in kudu hms Dryrun
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18280 ) Change subject: [tools] KUDU- Include Table Counts in kudu hms Dryrun .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18280 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idf26141d2a3fd6cbb7249b3492fc6a50a0c0aa2d Gerrit-Change-Number: 18280 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 17 Mar 2022 20:10:47 + Gerrit-HasComments: No
[kudu-CR] [tool] Fix rebuild master tool's help infomation
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18319 ) Change subject: [tool] Fix rebuild master tool's help infomation .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18319 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I301df64f0c1a42a00f19bced18f13aa5057b068c Gerrit-Change-Number: 18319 Gerrit-PatchSet: 2 Gerrit-Owner: Yuqi Du Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Comment-Date: Wed, 16 Mar 2022 21:11:45 + Gerrit-HasComments: No
[kudu-CR] [tool] Fix rebuild master tool's help infomation
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18319 ) Change subject: [tool] Fix rebuild master tool's help infomation .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/18319/1/src/kudu/tools/tool_action_master.cc File src/kudu/tools/tool_action_master.cc: http://gerrit.cloudera.org:8080/#/c/18319/1/src/kudu/tools/tool_action_master.cc@117 PS1, Line 117: "of nit: missing space -- To view, visit http://gerrit.cloudera.org:8080/18319 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I301df64f0c1a42a00f19bced18f13aa5057b068c Gerrit-Change-Number: 18319 Gerrit-PatchSet: 1 Gerrit-Owner: Yuqi Du Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 14 Mar 2022 22:34:36 + Gerrit-HasComments: Yes
[kudu-CR] [tools] KUDU-3333 Table Counts in kudu hms dryrun
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18280 ) Change subject: [tools] KUDU- Table Counts in kudu hms dryrun .. Patch Set 1: Code-Review+1 We chatted about this a bit offline. Would be nice to add snippets of the before and after output to the commit message, since we're not adding a test in this patch. -- To view, visit http://gerrit.cloudera.org:8080/18280 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idf26141d2a3fd6cbb7249b3492fc6a50a0c0aa2d Gerrit-Change-Number: 18280 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 14 Mar 2022 22:16:48 + Gerrit-HasComments: No
[kudu-CR] [tablet] add tablet id into trace of a slow write op
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18315 ) Change subject: [tablet] add tablet_id into trace of a slow write op .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18315 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I06b05e8be1f5376b60244228809be9d260ed8398 Gerrit-Change-Number: 18315 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 14 Mar 2022 22:14:28 + Gerrit-HasComments: No
[kudu-CR] [tools] run intra-location rebalancing in parallel
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/18308 )
Change subject: [tools] run intra-location rebalancing in parallel
..
Patch Set 1: Code-Review+1
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18308/1/src/kudu/tools/rebalancer_tool.cc
File src/kudu/tools/rebalancer_tool.cc:
http://gerrit.cloudera.org:8080/#/c/18308/1/src/kudu/tools/rebalancer_tool.cc@324
PS1, Line 324: s.message()
nit: do you think it's worth adding information about which location had which
status? Is that easy to figure out from the logs?
http://gerrit.cloudera.org:8080/#/c/18308/1/src/kudu/tools/rebalancer_tool.cc@328
PS1, Line 328: if (!status.ok()) {
: return status;
: }
nit: RETURN_NOT_OK(status)?
http://gerrit.cloudera.org:8080/#/c/18308/1/src/kudu/tools/rebalancer_tool.cc@737
PS1, Line 737: guard
nit: maybe name this refresh_lock so it's less error prone (e.g. easy to
conflate with the held ksck_lock below
--
To view, visit http://gerrit.cloudera.org:8080/18308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4fe3ef3ec2fcac57114c97d5b6cd81d5d9953c4
Gerrit-Change-Number: 18308
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 14 Mar 2022 22:13:45 +
Gerrit-HasComments: Yes
[kudu-CR] [tools] Support copying unpartitioned table
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18318 ) Change subject: [tools] Support copying unpartitioned table .. Patch Set 1: Code-Review+2 Thanks for fixing this! -- To view, visit http://gerrit.cloudera.org:8080/18318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4fcdcf93aadfaa2df59e59afa7bb3524ede2ac60 Gerrit-Change-Number: 18318 Gerrit-PatchSet: 1 Gerrit-Owner: Yifan Zhang Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Mon, 14 Mar 2022 21:26:38 + Gerrit-HasComments: No
[kudu-CR] [tablet] output more info for long row lock waits
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18300 ) Change subject: [tablet] output more info for long row lock waits .. Patch Set 1: Code-Review+2 Thanks for addressing this! -- To view, visit http://gerrit.cloudera.org:8080/18300 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I521976ff6986676ad9a33a810c56eaa8e7deeab8 Gerrit-Change-Number: 18300 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 09 Mar 2022 20:48:02 + Gerrit-HasComments: No
[kudu-CR] [tools] correct rebalancer's initial status message
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18297 ) Change subject: [tools] correct rebalancer's initial status message .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18297 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic224a07f9ce9ed3ca9a447022f183b799a1bebee Gerrit-Change-Number: 18297 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 08 Mar 2022 22:20:21 + Gerrit-HasComments: No
[kudu-CR] KUDU-3197 [tserver] optimal Schema's memory used, using std::shared ptr
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18255 ) Change subject: KUDU-3197 [tserver] optimal Schema's memory used, using std::shared_ptr .. Patch Set 13: Thanks for the contribution! -- To view, visit http://gerrit.cloudera.org:8080/18255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic284dde108c49130419d876c6698b40c195e9b35 Gerrit-Change-Number: 18255 Gerrit-PatchSet: 13 Gerrit-Owner: Yuqi Du Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Comment-Date: Fri, 04 Mar 2022 00:17:02 + Gerrit-HasComments: No
[kudu-CR] [thirdparty] build curl without brotli
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18288 ) Change subject: [thirdparty] build curl without brotli .. Patch Set 2: > Patch Set 2: > > > Patch Set 2: > > > > > Curious, why was this problematic before? Or is this more a hygiene > > > improvement, build only what we use? > > > > This becomes an issue only when brotli is installed on the build machine. > > This becomes a problem only for RELEASE builds (statically linked > > binaries). In case of dynamically linked binaries, that's not visible. > > > > Did you happen to have broli compression libraries on your laptop or build > > machine discovered by standard means? If so, try build RELEASE build -- > > I'm curious whether it's something specific to my environment, by from the > > configuration script I can see that it has --with-broli by default. > > Ah, I haven't built RELEASE binaries in some time. I'll try that out. Indeed, this led to issues for me too. Thanks for the fix! -- To view, visit http://gerrit.cloudera.org:8080/18288 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I14ef696302cc1653f3efd6dba5714ce3927c1d20 Gerrit-Change-Number: 18288 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Thu, 03 Mar 2022 23:51:45 + Gerrit-HasComments: No
[kudu-CR] KUDU-3197 [tserver] optimal Schema's memory used, using std::shared ptr
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18255 ) Change subject: KUDU-3197 [tserver] optimal Schema's memory used, using std::shared_ptr .. KUDU-3197 [tserver] optimal Schema's memory used, using std::shared_ptr Change TabletMeta's variable Schema* to std::shared_ptr to reduce memory used when alter schema. Because TabletMeta save old_schemas to reserve the elder schemas when alter schema, maybe they have been used by scanners or compaction jobs. As jira KUDU-3197 said, frequently alter schema will lead to tserver's memory becomes very large, just like memory leak, especially column's number is very large. The jira issued by wangningito, and I continue his work, and now use std::shared_ptr instead of scoped_refptr, because scoped_refptr causes too many changes, just as: https://gerrit.cloudera.org/c/18098/ Change-Id: Ic284dde108c49130419d876c6698b40c195e9b35 Reviewed-on: http://gerrit.cloudera.org:8080/18255 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M src/kudu/client/client-test.cc M src/kudu/common/schema.h M src/kudu/integration-tests/linked_list-test-util.h M src/kudu/master/sys_catalog.cc M src/kudu/tablet/all_types-scan-correctness-test.cc M src/kudu/tablet/cfile_set.cc M src/kudu/tablet/cfile_set.h M src/kudu/tablet/diff_scan-test.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/mt-tablet-test.cc M src/kudu/tablet/ops/alter_schema_op.cc M src/kudu/tablet/ops/alter_schema_op.h M src/kudu/tablet/ops/op.h M src/kudu/tablet/ops/write_op.h M src/kudu/tablet/rowset_metadata.h M src/kudu/tablet/tablet-schema-test.cc M src/kudu/tablet/tablet-test-util.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tablet/tablet_metadata.h M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tserver/scanners.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tserver_path_handlers.cc 30 files changed, 161 insertions(+), 149 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/18255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ic284dde108c49130419d876c6698b40c195e9b35 Gerrit-Change-Number: 18255 Gerrit-PatchSet: 13 Gerrit-Owner: Yuqi Du Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du
[kudu-CR] KUDU-3197 [tserver] optimal Schema's memory used, using std::shared ptr
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18255 ) Change subject: KUDU-3197 [tserver] optimal Schema's memory used, using std::shared_ptr .. Patch Set 12: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic284dde108c49130419d876c6698b40c195e9b35 Gerrit-Change-Number: 18255 Gerrit-PatchSet: 12 Gerrit-Owner: Yuqi Du Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Comment-Date: Thu, 03 Mar 2022 23:52:54 + Gerrit-HasComments: No
[kudu-CR] [thirdparty] build curl without brotli
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18288 ) Change subject: [thirdparty] build curl without brotli .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18288 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I14ef696302cc1653f3efd6dba5714ce3927c1d20 Gerrit-Change-Number: 18288 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Thu, 03 Mar 2022 23:51:11 + Gerrit-HasComments: No
[kudu-CR] [thirdparty] build curl without brotli
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18288 ) Change subject: [thirdparty] build curl without brotli .. Patch Set 2: > Patch Set 2: > > > Curious, why was this problematic before? Or is this more a hygiene > > improvement, build only what we use? > > This becomes an issue only when brotli is installed on the build machine. > This becomes a problem only for RELEASE builds (statically linked binaries). > In case of dynamically linked binaries, that's not visible. > > Did you happen to have broli compression libraries on your laptop or build > machine discovered by standard means? If so, try build RELEASE build -- I'm > curious whether it's something specific to my environment, by from the > configuration script I can see that it has --with-broli by default. Ah, I haven't built RELEASE binaries in some time. I'll try that out. -- To view, visit http://gerrit.cloudera.org:8080/18288 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I14ef696302cc1653f3efd6dba5714ce3927c1d20 Gerrit-Change-Number: 18288 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Thu, 03 Mar 2022 23:15:46 + Gerrit-HasComments: No
[kudu-CR] [thirdparty] build curl without brotli
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18288 ) Change subject: [thirdparty] build curl without brotli .. Patch Set 2: Code-Review+1 Curious, why was this problematic before? Or is this more a hygiene improvement, build only what we use? -- To view, visit http://gerrit.cloudera.org:8080/18288 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I14ef696302cc1653f3efd6dba5714ce3927c1d20 Gerrit-Change-Number: 18288 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Thu, 03 Mar 2022 22:58:00 + Gerrit-HasComments: No
[kudu-CR](branch-1.16.x) [docs] Add release notes for 1.16.0
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18275 ) Change subject: [docs] Add release notes for 1.16.0 .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.16.x Gerrit-MessageType: comment Gerrit-Change-Id: I0beb2e182af261ce785cfe21c7de34ca953e6a32 Gerrit-Change-Number: 18275 Gerrit-PatchSet: 9 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hongjiang Zhang Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Khazar Mammadli Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Will Berkeley Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: yejiabao Gerrit-Comment-Date: Wed, 02 Mar 2022 17:50:19 + Gerrit-HasComments: No
[kudu-CR] KUDU-3197 [tserver] optimal Schema's memory used, using std::shared ptr
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18255 ) Change subject: KUDU-3197 [tserver] optimal Schema's memory used, using std::shared_ptr .. Patch Set 8: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/18255/8/src/kudu/tablet/tablet_metadata.h File src/kudu/tablet/tablet_metadata.h: http://gerrit.cloudera.org:8080/#/c/18255/8/src/kudu/tablet/tablet_metadata.h@379 PS8, Line 379: Set nit: maybe remove the Set here, so it's just SwapSchemaUnlocked http://gerrit.cloudera.org:8080/#/c/18255/8/src/kudu/tablet/tablet_metadata.h@447 PS8, Line 447: // We don't use unique_ptr so that we can do an atomic swap. nit: I'm not sure what this means. There doesn't seem to be much of a difference between swapping a unique_ptr vs a shared_ptr. -- To view, visit http://gerrit.cloudera.org:8080/18255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic284dde108c49130419d876c6698b40c195e9b35 Gerrit-Change-Number: 18255 Gerrit-PatchSet: 8 Gerrit-Owner: Yuqi Du Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Comment-Date: Wed, 02 Mar 2022 06:42:33 + Gerrit-HasComments: Yes
[kudu-CR](branch-1.16.x) [docs] Add release notes for 1.16.0
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18275 ) Change subject: [docs] Add release notes for 1.16.0 .. Patch Set 8: (3 comments) http://gerrit.cloudera.org:8080/#/c/18275/3/docs/release_notes.adoc File docs/release_notes.adoc: http://gerrit.cloudera.org:8080/#/c/18275/3/docs/release_notes.adoc@66 PS3, Line 66: * It’s now possible to track startup progress on the `/startup` page on the web : UI (link:https://issues.apache.org/jira/browse/KUDU-1959[KUDU-1959 > Done Seems this was reverted in revision 8 http://gerrit.cloudera.org:8080/#/c/18275/6/docs/release_notes.adoc File docs/release_notes.adoc: http://gerrit.cloudera.org:8080/#/c/18275/6/docs/release_notes.adoc@117 PS6, Line 117: > Yep, thanks for pointing this out, not sure why I wrote cmeta instead of PB Seems this was reverted in revision 8 http://gerrit.cloudera.org:8080/#/c/18275/6/docs/release_notes.adoc@130 PS6, Line 130: ira/browse/KUDU-3311[KUDU-3311]). : > Done Seems this was reverted in revision 8 -- To view, visit http://gerrit.cloudera.org:8080/18275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.16.x Gerrit-MessageType: comment Gerrit-Change-Id: I0beb2e182af261ce785cfe21c7de34ca953e6a32 Gerrit-Change-Number: 18275 Gerrit-PatchSet: 8 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hongjiang Zhang Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Khazar Mammadli Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Will Berkeley Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: yejiabao Gerrit-Comment-Date: Wed, 02 Mar 2022 00:26:12 + Gerrit-HasComments: Yes
[kudu-CR] [security] avoid allocating string in SimpleAcl::UserAllowed()
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18283 ) Change subject: [security] avoid allocating string in SimpleAcl::UserAllowed() .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/18283/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18283/1//COMMIT_MSG@27 PS1, Line 27: std::string : Interesting -- it makes sense, but for some reason it's still surprising such a simple type mismatch (std::string vs const char*) makes such a difference. Neat, and potentially scary (I wonder if this shows up anywhere else)... -- To view, visit http://gerrit.cloudera.org:8080/18283 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic9ed6140cbcf0487ec76b9f769cdf24f9de7b52c Gerrit-Change-Number: 18283 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 01 Mar 2022 02:07:19 + Gerrit-HasComments: Yes
[kudu-CR] [tablet] don't call UpdateMetricsForOp() for rows with errors
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18281 ) Change subject: [tablet] don't call UpdateMetricsForOp() for rows with errors .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18281 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic1f57ee7d1b0064569a34ba93d35979426f76812 Gerrit-Change-Number: 18281 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 01 Mar 2022 01:05:19 + Gerrit-HasComments: No
[kudu-CR] [tablet] don't call UpdateMetricsForOp() for rows with errors
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18281 ) Change subject: [tablet] don't call UpdateMetricsForOp() for rows with errors .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/18281/1/src/kudu/tablet/ops/write_op.cc File src/kudu/tablet/ops/write_op.cc: http://gerrit.cloudera.org:8080/#/c/18281/1/src/kudu/tablet/ops/write_op.cc@250 PS1, Line 250: !op->result->has_failed_status() nit: maybe put this in an else block to avoid the negative? -- To view, visit http://gerrit.cloudera.org:8080/18281 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic1f57ee7d1b0064569a34ba93d35979426f76812 Gerrit-Change-Number: 18281 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 28 Feb 2022 23:43:15 + Gerrit-HasComments: Yes
[kudu-CR](branch-1.16.x) [docs] Add release notes for 1.16.0
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18275 ) Change subject: [docs] Add release notes for 1.16.0 .. Patch Set 6: Code-Review+1 (3 comments) http://gerrit.cloudera.org:8080/#/c/18275/6/docs/release_notes.adoc File docs/release_notes.adoc: http://gerrit.cloudera.org:8080/#/c/18275/6/docs/release_notes.adoc@92 PS6, Line 92: --row_count_only nit: surround in back-ticks. Same elsewhere for consistency http://gerrit.cloudera.org:8080/#/c/18275/6/docs/release_notes.adoc@117 PS6, Line 117: cmeta Is this referring to this patch[1]? It shouldn't be specific to cmeta files, I don't think [1] https://gerrit.cloudera.org/c/17808/ http://gerrit.cloudera.org:8080/#/c/18275/6/docs/release_notes.adoc@130 PS6, Line 130: cluster with if +1 master address is present : in the flags nit: how about "... start up a master when there is an additional master address present in the master addresses flag." -- To view, visit http://gerrit.cloudera.org:8080/18275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.16.x Gerrit-MessageType: comment Gerrit-Change-Id: I0beb2e182af261ce785cfe21c7de34ca953e6a32 Gerrit-Change-Number: 18275 Gerrit-PatchSet: 6 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hongjiang Zhang Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Khazar Mammadli Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Will Berkeley Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: yejiabao Gerrit-Comment-Date: Mon, 28 Feb 2022 21:38:38 + Gerrit-HasComments: Yes
[kudu-CR](branch-1.16.x) [docs] add blurb about automatic master addition
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18279 ) Change subject: [docs] add blurb about automatic master addition .. [docs] add blurb about automatic master addition The steps are updated in the latest version of Kudu, and this patch adds a note indicating to readers that they do not need to go through the administrative docs. Change-Id: I15ada3ce0a4319a1e1b5340bdfb7a261ef260f8d Reviewed-on: http://gerrit.cloudera.org:8080/18278 Tested-by: Kudu Jenkins Reviewed-by: Attila Bukor (cherry picked from commit cb016a09994f4b6cf89e62f76b5a5b4bc2a0a3ad) Reviewed-on: http://gerrit.cloudera.org:8080/18279 --- M docs/administration.adoc 1 file changed, 7 insertions(+), 3 deletions(-) Approvals: Kudu Jenkins: Verified Attila Bukor: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/18279 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.16.x Gerrit-MessageType: merged Gerrit-Change-Id: I15ada3ce0a4319a1e1b5340bdfb7a261ef260f8d Gerrit-Change-Number: 18279 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR](branch-1.16.x) [docs] Add release notes for 1.16.0
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18275 ) Change subject: [docs] Add release notes for 1.16.0 .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/18275/3/docs/release_notes.adoc File docs/release_notes.adoc: http://gerrit.cloudera.org:8080/#/c/18275/3/docs/release_notes.adoc@147 PS3, Line 147: vd.nist > Ah, for some reason I thought we relied on users to bring their own logger. For instance, in our Java examples, we depend on slf4j-simple, rather than using log4j[1]. And it seems we only testCompile log4j everywhere except the subprocess[2]. Though I might be missing something -- I'm definitely no Java build expert [1] https://github.com/apache/kudu/blob/master/examples/java/java-example/pom.xml#L83 [2] https://github.com/apache/kudu/search?l=Gradle=log4j -- To view, visit http://gerrit.cloudera.org:8080/18275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.16.x Gerrit-MessageType: comment Gerrit-Change-Id: I0beb2e182af261ce785cfe21c7de34ca953e6a32 Gerrit-Change-Number: 18275 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hongjiang Zhang Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Khazar Mammadli Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Will Berkeley Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: yejiabao Gerrit-Comment-Date: Sat, 26 Feb 2022 03:36:59 + Gerrit-HasComments: Yes
[kudu-CR](branch-1.16.x) [docs] Add release notes for 1.16.0
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18275 ) Change subject: [docs] Add release notes for 1.16.0 .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/18275/3/docs/release_notes.adoc File docs/release_notes.adoc: http://gerrit.cloudera.org:8080/#/c/18275/3/docs/release_notes.adoc@147 PS3, Line 147: vd.nist > SLF4J is only an abstraction layer, the underlying is still Log4J. Ah, for some reason I thought we relied on users to bring their own logger. -- To view, visit http://gerrit.cloudera.org:8080/18275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.16.x Gerrit-MessageType: comment Gerrit-Change-Id: I0beb2e182af261ce785cfe21c7de34ca953e6a32 Gerrit-Change-Number: 18275 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hongjiang Zhang Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Khazar Mammadli Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Will Berkeley Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: yejiabao Gerrit-Comment-Date: Sat, 26 Feb 2022 00:41:50 + Gerrit-HasComments: Yes
[kudu-CR] [docs] add blurb about automatic master addition
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18278 ) Change subject: [docs] add blurb about automatic master addition .. Patch Set 2: Done. Thanks for the review! -- To view, visit http://gerrit.cloudera.org:8080/18278 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I15ada3ce0a4319a1e1b5340bdfb7a261ef260f8d Gerrit-Change-Number: 18278 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Feb 2022 23:02:01 + Gerrit-HasComments: No
[kudu-CR](branch-1.16.x) [docs] add blurb about automatic master addition
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18279 Change subject: [docs] add blurb about automatic master addition .. [docs] add blurb about automatic master addition The steps are updated in the latest version of Kudu, and this patch adds a note indicating to readers that they do not need to go through the administrative docs. Change-Id: I15ada3ce0a4319a1e1b5340bdfb7a261ef260f8d Reviewed-on: http://gerrit.cloudera.org:8080/18278 Tested-by: Kudu Jenkins Reviewed-by: Attila Bukor (cherry picked from commit cb016a09994f4b6cf89e62f76b5a5b4bc2a0a3ad) --- M docs/administration.adoc 1 file changed, 7 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/18279/1 -- To view, visit http://gerrit.cloudera.org:8080/18279 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.16.x Gerrit-MessageType: newchange Gerrit-Change-Id: I15ada3ce0a4319a1e1b5340bdfb7a261ef260f8d Gerrit-Change-Number: 18279 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] [docs] add blurb about automatic master addition
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18278 ) Change subject: [docs] add blurb about automatic master addition .. [docs] add blurb about automatic master addition The steps are updated in the latest version of Kudu, and this patch adds a note indicating to readers that they do not need to go through the administrative docs. Change-Id: I15ada3ce0a4319a1e1b5340bdfb7a261ef260f8d Reviewed-on: http://gerrit.cloudera.org:8080/18278 Tested-by: Kudu Jenkins Reviewed-by: Attila Bukor --- M docs/administration.adoc 1 file changed, 7 insertions(+), 3 deletions(-) Approvals: Kudu Jenkins: Verified Attila Bukor: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/18278 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I15ada3ce0a4319a1e1b5340bdfb7a261ef260f8d Gerrit-Change-Number: 18278 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [docs] add blurb about automatic master addition
Hello Attila Bukor, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/18278 to review the following change. Change subject: [docs] add blurb about automatic master addition .. [docs] add blurb about automatic master addition The steps are updated in the latest version of Kudu, and this patch adds a note indicating to readers that they do not need to go through the administrative docs. Change-Id: I15ada3ce0a4319a1e1b5340bdfb7a261ef260f8d --- M docs/administration.adoc 1 file changed, 7 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/78/18278/1 -- To view, visit http://gerrit.cloudera.org:8080/18278 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I15ada3ce0a4319a1e1b5340bdfb7a261ef260f8d Gerrit-Change-Number: 18278 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Attila Bukor
[kudu-CR] [security] handle a few unexpected authn token conditions
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18271 ) Change subject: [security] handle a few unexpected authn token conditions .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18271 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic05ff6a9b289877d8440b94f00b2375da938c901 Gerrit-Change-Number: 18271 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 25 Feb 2022 22:10:43 + Gerrit-HasComments: No
[kudu-CR](branch-1.16.x) [docs] Add release notes for 1.16.0
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18275 ) Change subject: [docs] Add release notes for 1.16.0 .. Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/18275/3/docs/release_notes.adoc File docs/release_notes.adoc: http://gerrit.cloudera.org:8080/#/c/18275/3/docs/release_notes.adoc@66 PS3, Line 66: /startup nit: surround with back-ticks, as is done for `/healthz`? http://gerrit.cloudera.org:8080/#/c/18275/3/docs/release_notes.adoc@66 PS3, Line 66: * It’s now possible to track startup progress on the /startup page on the web UI : (link:https://issues.apache.org/jira/browse/KUDU-1959[KUDU-1959]). It's probably also worth mentioning the startup metrics. WDYT? cc Abhishek http://gerrit.cloudera.org:8080/#/c/18275/3/docs/release_notes.adoc@73 PS3, Line 73: * A new tool `kudu table set_replication_factor` is added to change the : replication factor of an existing table : (link:https://issues.apache.org/jira/browse/KUDU-3304[KUDU-3304]). This is a duplicate of L53? http://gerrit.cloudera.org:8080/#/c/18275/3/docs/release_notes.adoc@132 PS3, Line 132: `start_kudu.sh` This change applies to more than just this script, right? http://gerrit.cloudera.org:8080/#/c/18275/3/docs/release_notes.adoc@136 PS3, Line 136: KuduWriteOperation nit: surround in back-ticks? http://gerrit.cloudera.org:8080/#/c/18275/3/docs/release_notes.adoc@147 PS3, Line 147: clients Just clarifying, we don't actually depend on Log4j in clients do we? other than in tests? I thought we use slf4j -- To view, visit http://gerrit.cloudera.org:8080/18275 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.16.x Gerrit-MessageType: comment Gerrit-Change-Id: I0beb2e182af261ce785cfe21c7de34ca953e6a32 Gerrit-Change-Number: 18275 Gerrit-PatchSet: 3 Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hongjiang Zhang Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Khazar Mammadli Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Will Berkeley Gerrit-Reviewer: Yifan Zhang Gerrit-Reviewer: Yingchun Lai Gerrit-Reviewer: Yuqi Du Gerrit-Reviewer: Zoltan Chovan Gerrit-Reviewer: yejiabao Gerrit-Comment-Date: Fri, 25 Feb 2022 22:05:10 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3197 [tserver] optimal Schema's memory used, using std::shared ptr
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/18255 )
Change subject: KUDU-3197 [tserver] optimal Schema's memory used, using
std::shared_ptr
..
Patch Set 5:
(5 comments)
Sorry for the delay in review. Thanks for the contribution! Looking good so far
:)
http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:
http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/diskrowset.cc@633
PS3, Line 633: const Schema* schema = rowset_metadata_->tablet_schema().get();
Do we need to make a shared_ptr here? I suspect we do
http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/ops/alter_schema_op.cc
File src/kudu/tablet/ops/alter_schema_op.cc:
http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/ops/alter_schema_op.cc@107
PS3, Line 107: schema_ptr.get())
Why not pass the shared ptr? Otherwise we end up creating a new shared ptr in
this call anyway.
http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/tablet.cc@548
PS3, Line 548: client_schema,
> Why there are two styles, one is declear a new variable, like 'SchemaPtr sc
+1, it's worth clarifying what the policy is for using shared_ptr vs raw
pointers.
For instances where the function expects a reference, I'm not sure it's safe to
just pass the pointer, since it's possible the underlying schema is destroyed,
since this call site doesn't keep a shared_ptr around. It'd be safer if we did
something like:
SchemaPtr shared_schema = schema();
RowOperationsPBDecoder dec(_state->request()->row_operations(),
client_schema,
schema_ptr.get(),...
Or perhaps you're relying on the fact that we locked the schema at L538? If so,
it's still worth in a comment, so other developers in this area of code better
understand the nuances.
http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:
http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tablet/tablet_metadata.cc@940
PS3, Line 940: std::lock_guard l(data_lock_);
: SetSchemaUnlocked(schema, version);
This may actually destruct the old schema, which could be expensive. Instead,
maybe we should implement some SwapSchemaUnlocked() so the destruction doesn't
happen while data_lock_ is held.
void TabletMetadata::SetSchema(SchemaPtr schema, uint32_t version) {
// In case this is the last reference to the schema, destruct the pointer
// outside the lock.
SchemaPtr old_schema;
{
lock_guard l(data_lock_);
SwapSchemaUnlocked(std::move(schema), version, _schema);
}
}
void TabletMetadata::SwapSchemaUnlocked(SchemaPtr schema, uint32_t version,
SchemaPtr* old_schema) {
*old_schema = std::move(schema_);
schema_ = std::move(schema);
schema_version_ = version;
}
http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tserver/tserver_path_handlers.cc
File src/kudu/tserver/tserver_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/18255/3/src/kudu/tserver/tserver_path_handlers.cc@368
PS3, Line 368: *tmeta->schema()
Don't we need to make a schema_ptr here?
--
To view, visit http://gerrit.cloudera.org:8080/18255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic284dde108c49130419d876c6698b40c195e9b35
Gerrit-Change-Number: 18255
Gerrit-PatchSet: 5
Gerrit-Owner: Yuqi Du
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai
Gerrit-Comment-Date: Fri, 25 Feb 2022 03:09:18 +
Gerrit-HasComments: Yes
[kudu-CR] [tests] harmonize assertions on the expected status
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18269 ) Change subject: [tests] harmonize assertions on the expected status .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18269 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09cba638a5328fc57cc1ca70196bab42f09569b4 Gerrit-Change-Number: 18269 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Thu, 24 Feb 2022 01:23:14 + Gerrit-HasComments: No
[kudu-CR](branch-1.16.x) [tool] fix a command bug, cmd: kudu wal dump ...
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18260 ) Change subject: [tool] fix a command bug, cmd: kudu wal dump ... .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.16.x Gerrit-MessageType: comment Gerrit-Change-Id: I27acc71597d038cafbbe687117bddb1ce16576c0 Gerrit-Change-Number: 18260 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yuqi Du Gerrit-Comment-Date: Mon, 21 Feb 2022 21:20:44 + Gerrit-HasComments: No
[kudu-CR] [webserver-test] unify generation of the server's URL
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18254 ) Change subject: [webserver-test] unify generation of the server's URL .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/18254 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic1e2536b2f548df9f28131368a5a8d1f46cd3053 Gerrit-Change-Number: 18254 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Fri, 18 Feb 2022 06:17:13 + Gerrit-HasComments: No
[kudu-CR] [webserver] add HSTS header for HTTPS responses
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18253 ) Change subject: [webserver] add HSTS header for HTTPS responses .. Patch Set 2: Code-Review+1 (3 comments) Thanks for addressing this! http://gerrit.cloudera.org:8080/#/c/18253/2/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: http://gerrit.cloudera.org:8080/#/c/18253/2/src/kudu/server/webserver.cc@93 PS2, Line 93: 31536000 nit: maybe 365 * 24 * 60 * 60 is also more easily verifiable? http://gerrit.cloudera.org:8080/#/c/18253/2/src/kudu/server/webserver.cc@94 PS2, Line 94: This is to enforce nit: maybe describe what the value is? Maybe something like: The time, in seconds, that a browser should remember that the webserver can only to be accessed using HTTPS. (just looking at https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Strict-Transport-Security) http://gerrit.cloudera.org:8080/#/c/18253/2/src/kudu/server/webserver.cc@98 PS2, Line 98: g nit: is set -- To view, visit http://gerrit.cloudera.org:8080/18253 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id844b9588196b3d608765d0f16f5caec1c414d41 Gerrit-Change-Number: 18253 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 18 Feb 2022 06:16:23 + Gerrit-HasComments: Yes
[kudu-CR] [util] harmonize random util
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18248 ) Change subject: [util] harmonize random_util .. Patch Set 2: > Patch Set 1: Verified+1 > > For some reason, IWYU wants "kudu/util/int128.h" to be included along with > "kudu/util/int128_util.h" in random_util-test.cc. Done. Thanks for the fast reviews! -- To view, visit http://gerrit.cloudera.org:8080/18248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I247d68bedbd021aa59b8f057e0eed3166c08ac33 Gerrit-Change-Number: 18248 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 18 Feb 2022 01:03:49 + Gerrit-HasComments: No
[kudu-CR] [util] harmonize random util
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/18248 to look at the new patch set (#2). Change subject: [util] harmonize random_util .. [util] harmonize random_util Functions defined in int128_util.h caused ambiguous overload errors for: std::ostream& operator<<(std::ostream& os, const unsigned __int128& val), This patch includes "kudu/util/int128.h" instead of "kudu/util/int128_util.h". Change-Id: I247d68bedbd021aa59b8f057e0eed3166c08ac33 --- M src/kudu/util/random_util-test.cc M src/kudu/util/random_util.h 2 files changed, 2 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/18248/2 -- To view, visit http://gerrit.cloudera.org:8080/18248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I247d68bedbd021aa59b8f057e0eed3166c08ac33 Gerrit-Change-Number: 18248 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] harmonize random util
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18248 Change subject: [util] harmonize random_util .. [util] harmonize random_util Functions defined in int128_util.h caused ambiguous overload errors for: std::ostream& operator<<(std::ostream& os, const unsigned __int128& val), This patch includes "kudu/util/int128.h" instead of "kudu/util/int128_util.h". Change-Id: I247d68bedbd021aa59b8f057e0eed3166c08ac33 --- M src/kudu/util/random_util-test.cc M src/kudu/util/random_util.h 2 files changed, 2 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/18248/1 -- To view, visit http://gerrit.cloudera.org:8080/18248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I247d68bedbd021aa59b8f057e0eed3166c08ac33 Gerrit-Change-Number: 18248 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] [util] harmonize logging.h
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18247 Change subject: [util] harmonize logging.h .. [util] harmonize logging.h This adds the ::kudu namespace to logging::LogThrottler, to match changes required for Impala to use the library. Change-Id: Ie3c6a4597cda3077df3374e907eb8672d279d9e9 --- M src/kudu/util/logging.h 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/18247/1 -- To view, visit http://gerrit.cloudera.org:8080/18247 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ie3c6a4597cda3077df3374e907eb8672d279d9e9 Gerrit-Change-Number: 18247 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] [security] harmonize tls handshake.cc
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18246 Change subject: [security] harmonize tls_handshake.cc .. [security] harmonize tls_handshake.cc The first parameter of Socket.Write() is of type `const uint8_t *`. This resulted in a warning in TlsHandshake::Finish(). This caused an Impala build failure since the warning was treated as an error. This is a follow-up to df6590d26d. Change-Id: Iddf1bd7ee39ea74eff74871c83c5159e53fc7377 --- M src/kudu/security/tls_handshake.cc 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/18246/1 -- To view, visit http://gerrit.cloudera.org:8080/18246 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iddf1bd7ee39ea74eff74871c83c5159e53fc7377 Gerrit-Change-Number: 18246 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] [security] harmonize updates to mini kdc
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18245 Change subject: [security] harmonize updates to mini_kdc .. [security] harmonize updates to mini_kdc Impala recently rebased their Kudu libraries and ran into some build issues. This patch addresses one: An Impala backend test calls MiniKdc::Kinit() with parameter "dummy/host", but "/" caused a runtime error in MiniKdc::Kinit(). This patch replaces "/" with "_" when creating the template. The same is not done for principal names, which can consist "/". This is a follow-up to 8133fbfb734c18be8f4abb3ee83ac9dd0cc4ce16. Change-Id: I60bd9a0bf3113a689a252182b4e6292dff12a9e7 --- M src/kudu/security/test/mini_kdc.cc 1 file changed, 3 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/18245/1 -- To view, visit http://gerrit.cloudera.org:8080/18245 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I60bd9a0bf3113a689a252182b4e6292dff12a9e7 Gerrit-Change-Number: 18245 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] [master] fix rare crash on master shutdown
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18239 ) Change subject: [master] fix rare crash on master shutdown .. Patch Set 1: Code-Review+2 Thank you for fixing this! -- To view, visit http://gerrit.cloudera.org:8080/18239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I006a4d617cd1cb21312dac2f467fa3d04060b866 Gerrit-Change-Number: 18239 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 17 Feb 2022 02:47:09 + Gerrit-HasComments: No
[kudu-CR] [tserver] avoid copying Schema in SplitKeyRange()
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18238 ) Change subject: [tserver] avoid copying Schema in SplitKeyRange() .. Patch Set 1: Verified+1 Code-Review+2 The test failures seem unrelated, though perhaps this one's related to the recent Thread change? threadpool.cc:306] Check failed: 1 == tokens_.size() (1 vs. 3) Threadpool raft destroyed with 3 allocated tokens @ 0x477930 __tsan::CallUserSignalHandler() at /home/jenkins-slave/workspace/kudu-master/2/thirdparty/src/llvm-9.0.0.src/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:1916 @ 0x479e47 rtl_sigaction() at /home/jenkins-slave/workspace/kudu-master/2/thirdparty/src/llvm-9.0.0.src/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:2006 @ 0x7f81497f3fb7 gsignal at ??:0 @ 0x7f81497f5921 abort at ??:0 @ 0x480f66 abort at /home/jenkins-slave/workspace/kudu-master/2/thirdparty/src/llvm-9.0.0.src/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:261 @ 0x7f814c979e47 kudu::ThreadPool::~ThreadPool() at ??:0 @ 0x7f8153c3854f std::__1::default_delete<>::operator()() at ??:0 @ 0x7f8153c384be std::__1::unique_ptr<>::reset() at ??:0 @ 0x7f8153bf7cfc std::__1::unique_ptr<>::~unique_ptr() at ??:0 @ 0x7f8153c98b54 kudu::kserver::KuduServer::~KuduServer() at ??:0 @ 0x7f8153c916eb kudu::master::Master::~Master() at ??:0 @ 0x7f8153c9199a kudu::master::Master::~Master() at ??:0 @ 0x7f8153cc8c78 std::__1::default_delete<>::operator()() at ??:0 @ 0x7f8153cc1f9e std::__1::unique_ptr<>::reset() at ??:0 @ 0x7f8153cd8393 kudu::master::MiniMaster::Shutdown() at ??:0 @ 0x4ff669 kudu::tools::RemoteKsckTest_TestClusterWithLocation_Test::TestBody() at /home/jenkins-slave/workspace/kudu-master/2/src/kudu/tools/ksck_remote-test.cc:606 @ 0x7f8154951e1c RUN_ALL_TESTS() at ??:0 @ 0x7f8154950c6a main at ??:0 @ 0x7f81497d6bf7 __libc_start_main at ??:0 @ 0x4433aa _start at ??:? https://gerrit.cloudera.org/c/18229/ Looking through a bit, I don't think so though. Will dig a bit more. -- To view, visit http://gerrit.cloudera.org:8080/18238 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I07642a438a46812828ddd871534bdf985155 Gerrit-Change-Number: 18238 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 17 Feb 2022 01:10:48 + Gerrit-HasComments: No
[kudu-CR] [tserver] avoid copying Schema in SplitKeyRange()
Andrew Wong has removed a vote on this change. Change subject: [tserver] avoid copying Schema in SplitKeyRange() .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/18238 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I07642a438a46812828ddd871534bdf985155 Gerrit-Change-Number: 18238 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
