[kudu-CR] docs: improvements to transaction semantics
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/9235 ) Change subject: docs: improvements to transaction semantics .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/9235/1/docs/transaction_semantics.adoc File docs/transaction_semantics.adoc: http://gerrit.cloudera.org:8080/#/c/9235/1/docs/transaction_semantics.adoc@59 PS1, Line 59: with different levels of consistency or correctness guarantees. Scans can perform time-travel reads, i.e. > nit: is this over the col limit? (seems like it, but i might be wrong as I Done http://gerrit.cloudera.org:8080/#/c/9235/1/docs/transaction_semantics.adoc@181 PS1, Line 181: `READ_LATEST`:: This is the default read mode. The server takes a snapshot of : the MVCC state and proceeds with the read immediately. Reads in this mode only yield : 'Read Committed' isolation. > are you leaving the new mode out on purpose? Yeah, I am planning to do another patch to add it. This patch is only updating existing ones. http://gerrit.cloudera.org:8080/#/c/9235/1/docs/transaction_semantics.adoc@205 PS1, Line 205: Note > don't we have a special "note" marker, or am I mistaken? Done http://gerrit.cloudera.org:8080/#/c/9235/1/docs/transaction_semantics.adoc@222 PS1, Line 222: Writes > ahah, well caught :) :) -- To view, visit http://gerrit.cloudera.org:8080/9235 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I23a2751923a4214f52e6cdb233f7e3aeee207da2 Gerrit-Change-Number: 9235 Gerrit-PatchSet: 1 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 21 Feb 2018 07:43:33 + Gerrit-HasComments: Yes
[kudu-CR] docs: improvements to transaction semantics
Hello Alex Rodoni, David Ribeiro Alves, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9235 to look at the new patch set (#2). Change subject: docs: improvements to transaction semantics .. docs: improvements to transaction semantics Change-Id: I23a2751923a4214f52e6cdb233f7e3aeee207da2 --- M docs/transaction_semantics.adoc 1 file changed, 21 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/35/9235/2 -- To view, visit http://gerrit.cloudera.org:8080/9235 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I23a2751923a4214f52e6cdb233f7e3aeee207da2 Gerrit-Change-Number: 9235 Gerrit-PatchSet: 2 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
Hello Dan Burkert, David Ribeiro Alves, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8847 to look at the new patch set (#5). Change subject: KUDU-1704: add java client support for READ_YOUR_WRITES mode .. KUDU-1704: add java client support for READ_YOUR_WRITES mode Change-Id: I6239521c022147257859e399f55c6f3f945af465 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java 4 files changed, 196 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/8847/5 -- To view, visit http://gerrit.cloudera.org:8080/8847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6239521c022147257859e399f55c6f3f945af465 Gerrit-Change-Number: 8847 Gerrit-PatchSet: 5 Gerrit-Owner: Hao HaoGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2291 (part 6) stacks: use libunwind directly for stack tracing
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9319 to look at the new patch set (#7). Change subject: KUDU-2291 (part 6) stacks: use libunwind directly for stack tracing .. KUDU-2291 (part 6) stacks: use libunwind directly for stack tracing Previously we used glog's wrapper around libunwind for stack tracing. However that has a deficiency that it assumes that, process wide, only one thread can be inside libunwind at a time[1] It appears that this is left over from some very old versions of libunwind, or was already unnecessarily conservative. libunwind is meant to be thread safe, and we have tests that will trigger if it is not. This just extracts the function body of the glog function we were using and does the same work manually. Without this fix, the "collect from all the threads at the same time" code path resulted in most of the threads collecting an empty trace since they tried to call libunwind at the same time. [1] https://github.com/google/glog/issues/298 Change-Id: I3a53e55d7c4e7ee50bcac5b1e81267df56383634 --- M build-support/iwyu/iwyu.sh A build-support/iwyu/mappings/libunwind.imp M src/kudu/util/debug-util-test.cc M src/kudu/util/debug-util.cc M src/kudu/util/debug-util.h 5 files changed, 81 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/9319/7 -- To view, visit http://gerrit.cloudera.org:8080/9319 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3a53e55d7c4e7ee50bcac5b1e81267df56383634 Gerrit-Change-Number: 9319 Gerrit-PatchSet: 7 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2291 (part 5): allow collecting stack traces asynchronously
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9318 to look at the new patch set (#6). Change subject: KUDU-2291 (part 5): allow collecting stack traces asynchronously .. KUDU-2291 (part 5): allow collecting stack traces asynchronously This removes the global variable previously used to communicate between a stack trace collector and the stack trace target. The global variable had the downside of allowing only a single stack trace operation in flight at a time, which means that for use cases where we want to capture the trace of a bunch of threads, we needed to do so serially. For use cases like process-wide traces this could make debugging a bit tricky. For example, because of the skew in time between collecting the stack of thread A and the stack of thread B, it's possible that the traces could show them holding the same lock, which we know to be impossible. Getting the collection time as close as possible to "simultaneous" can reduce the possibility of these kind of strange-looking results. The actual mechanism to do this is slightly tricky. Rather than duplicate the full description here, check the comments in the implementation. In particular, StackTraceCollector::RevokeSigData() explains the complexity of not knowing whether our signal will ever be delivered. A new unit test shows the pattern, and this also updates the stack servlet to send the signals to all of the threads as close as possible to "the same time". Change-Id: Ie4d7789bb1272db033df4685f46eaf8483ec7be1 --- M src/kudu/server/default_path_handlers.cc M src/kudu/util/debug-util-test.cc M src/kudu/util/debug-util.cc M src/kudu/util/debug-util.h 4 files changed, 317 insertions(+), 109 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/9318/6 -- To view, visit http://gerrit.cloudera.org:8080/9318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie4d7789bb1272db033df4685f46eaf8483ec7be1 Gerrit-Change-Number: 9318 Gerrit-PatchSet: 6 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2297 (part 4): periodically dump stacks to diagnostics log
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9330 ) Change subject: KUDU-2297 (part 4): periodically dump stacks to diagnostics log .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/9330/5/src/kudu/server/diagnostics_log.cc File src/kudu/server/diagnostics_log.cc: http://gerrit.cloudera.org:8080/#/c/9330/5/src/kudu/server/diagnostics_log.cc@27 PS5, Line 27: #include > warning: #includes are not sorted properly [llvm-include-order] Done -- To view, visit http://gerrit.cloudera.org:8080/9330 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic32abf2c48ac6a5f3c384e58838b34671bcaf147 Gerrit-Change-Number: 9330 Gerrit-PatchSet: 5 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 21 Feb 2018 07:14:45 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2291 (part 5): allow collecting stack traces asynchronously
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9318 ) Change subject: KUDU-2291 (part 5): allow collecting stack traces asynchronously .. Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.h File src/kudu/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.h@208 PS4, Line 208: Trigger > TriggerAsync Done http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc File src/kudu/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@141 PS4, Line 141: void Signal() { > add: // Wakes all waiters, notifying them of completion. Done http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@151 PS4, Line 151: bool WaitUntil(MonoTime deadline) { > add: // Returns false if reached deadline before completion, true otherwise Done http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@216 PS4, Line 216: maybe > nit: capitalize Done http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@219 PS4, Line 219: ANNOTATE_HAPPENS_AFTER(sig); > why is this needed? Because we're using the signal data as a sort of queue to move the 'info' pointer between two threads, we need this to ensure that TSAN sees there is an enforced ordering between the two. Normally we get this ordering implied when thread A acquires a mutex or spinlock previously released by thread B, but in this case there is no such mutex. http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@230 PS4, Line 230: 2 > nit: /*skip_frames=*/ Done http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@312 PS4, Line 312: } > nit: move up one line Done http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@395 PS4, Line 395: SYS_rt_tgsigqueueinfo > Did you look into using pthread_sigqueue() ? I suppose a minor benefit of t yea, the problem is that for pthread_sigqueue we need to have pthread_t for all the threads, and best I can tell there is no way to go from a tid to a pthread_t. Additionally in the past I looked into how to list all running threads using pthreads APIs and came up with nothing particularly nice. There's some 'thread_db' API but it seems meant for debuggers and not easy to use at all. http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@396 PS4, Line 396: ) > , ErrnoToString(errno), errno) ? this ends up printed in the /stacks page and stuff, so I think it's better to just have the nice message here. http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@427 PS4, Line 427: Status StackTraceCollector::TriggerAsync(int64_t tid_, StackTrace* stack) { > missing void StackTraceCollector::RevokeSigData() for non-linux Done -- To view, visit http://gerrit.cloudera.org:8080/9318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie4d7789bb1272db033df4685f46eaf8483ec7be1 Gerrit-Change-Number: 9318 Gerrit-PatchSet: 4 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 21 Feb 2018 07:13:38 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2291 (part 4): avoid potential for deadlocks in stack trace collection
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9262 ) Change subject: KUDU-2291 (part 4): avoid potential for deadlocks in stack trace collection .. Patch Set 6: Code-Review+2 (1 comment) Carrying forward +2 http://gerrit.cloudera.org:8080/#/c/9262/5/src/kudu/util/debug/unwind_safeness.cc File src/kudu/util/debug/unwind_safeness.cc: http://gerrit.cloudera.org:8080/#/c/9262/5/src/kudu/util/debug/unwind_safeness.cc@23 PS5, Line 23: // Based on public domain code by Aliaksey Kandratsenka at > Do we need to add this to LICENSE? It's only super loosely based on it, so I don't think so (eg his original was ANSI C). Plus public domain has no reporting requirements by ASF policy AFAIK -- To view, visit http://gerrit.cloudera.org:8080/9262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I414718a26c4810de86a10397bb9803dc5e386d1a Gerrit-Change-Number: 9262 Gerrit-PatchSet: 6 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 21 Feb 2018 06:08:00 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2291 (part 4): avoid potential for deadlocks in stack trace collection
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9262 ) Change subject: KUDU-2291 (part 4): avoid potential for deadlocks in stack trace collection .. KUDU-2291 (part 4): avoid potential for deadlocks in stack trace collection This follows some recommendations from [1] to avoid the potential for deadlocks if we attempt to gather a stack trace from a signal handler context while the thread is inside calls to libdl. The approach is somewhat ugly: we have to override the sensitive libdl calls and use them to increment a thread-local counter indicating that it would be unsafe to collect a stack. We then check this counter before capturing a stack: if we see that we are inside libdl we just return an fake stack trace indicating why it could not be captured. The new test included deadlocks reliably without the fix. [1] https://github.com/gperftools/gperftools/wiki/gperftools'-stacktrace-capturing-methods-and-their-issues Change-Id: I414718a26c4810de86a10397bb9803dc5e386d1a Reviewed-on: http://gerrit.cloudera.org:8080/9262 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon--- M src/kudu/util/CMakeLists.txt M src/kudu/util/debug-util-test.cc M src/kudu/util/debug-util.cc M src/kudu/util/debug-util.h A src/kudu/util/debug/unwind_safeness.cc A src/kudu/util/debug/unwind_safeness.h 6 files changed, 291 insertions(+), 10 deletions(-) Approvals: Kudu Jenkins: Verified Todd Lipcon: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/9262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I414718a26c4810de86a10397bb9803dc5e386d1a Gerrit-Change-Number: 9262 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2291 (part 3): use futex to speed up stack collection
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9254 ) Change subject: KUDU-2291 (part 3): use futex to speed up stack collection .. KUDU-2291 (part 3): use futex to speed up stack collection Previously the thread which requests a stack trace would poll on an atomic variable waiting for the stack to be collected. Of course spinning is too CPU-hungry so it would actually sleep for 10ms in each iteration. This limited the performance of stack collection to about 100/second, which is problematic for the /stacks web page on a server that may have thousands of threads. This switches to using the futex system call to allow the dumper thread to sleep and the dumpee thread to wake it back up when the results are ready. Why do we use the low level futex and not something like a condition variable or CountdownLatch? The issue is that the dumpee thread is running in a signal handler context, and most things are not safe to do in that context. Particularly, mutexes may block, which is very naughty, and condition variables and latches are currently implemented on top of mutexes. pthread semaphores are another option, but those don't support proper timeout semantics, which is a feature we use in this use case. Whether the futex syscall itself is async-signal-safe is up for some debate. Clearly the "wait" mode is not, since it may block, but is the "wake" mode that we use here allowed? Technically I don't think so. In practice, the pthread sem_post() API _is_ documented to be async-signal-safe and I verified that in libc source it's implemented using futex to wake the waiter. So, if we are doing something illegal, at least we're doing the same illegal thing as libc, so we're very unlikely to ever break. Obviously futex is not portable to macOS, but this code was already Linux-specific so we haven't lost much. This speeds up the benchmark for stack trace collection (without symbolization) by 585x: Before: I0207 19:33:20.960477 30799 debug-util-test.cc:175] Throughput: 99 dumps/second (symbolize=0) I0207 19:33:21.963946 30799 debug-util-test.cc:175] Throughput: 84 dumps/second (symbolize=1) After: I0207 19:38:32.505213 32039 debug-util-test.cc:175] Throughput: 57916 dumps/second (symbolize=0) I0207 19:38:33.506417 32039 debug-util-test.cc:175] Throughput: 595 dumps/second (symbolize=1) Change-Id: Icb772f0607275ec340c7f38be05347fc14820cbd Reviewed-on: http://gerrit.cloudera.org:8080/9254 Reviewed-by: Mike PercyTested-by: Kudu Jenkins --- M src/kudu/util/debug-util.cc 1 file changed, 70 insertions(+), 16 deletions(-) Approvals: Mike Percy: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/9254 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Icb772f0607275ec340c7f38be05347fc14820cbd Gerrit-Change-Number: 9254 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-1704: add c++ client support for READ YOUR WRITES mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8823 ) Change subject: KUDU-1704: add c++ client support for READ_YOUR_WRITES mode .. Patch Set 10: (8 comments) http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-internal.h@197 PS9, Line 197: : // > this sentence is non-sensical. also max/latest is bound to be confusing. re Makes sense. Updated. http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-internal.h@201 PS9, Line 201: //errors, timeouts, or leadership issues. : // 3) 'deadline' (if initialized) elapses. : // : // NOTE: 'rpc_timeout' is a per-call timeout, while > also non-sensical; also see my comment above Done http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-test.cc@992 PS9, Line 992: ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 27, kNoBound)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 0, 15)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 0, 10)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 0, 20)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 0, 30)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 14, 30)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 30, 30)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, kTabletsNum * kRowsPerTablet, :kNoBound)); : } : INSTANTIATE_TEST_CASE_P(Params, ScanMultiTabletParamTest, : testing::ValuesIn(read_modes)); : : TEST_F(ClientTest, TestScanEmptyTable) { : K > can you make this a parameterized test that received the read mode as input Done http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client.h@1740 PS9, Line 1740: /// When @c READ_YOUR_WRITES is specified, the server will perform a server-local > I still think there is room to simplify these docs. I would prefer if they Done http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client.h@1743 PS9, Line 1743: minimize > nit: verbage (... ensures ... minimize sounds weird) Done http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client.h@1745 PS9, Line 1745: > What does this mean in terms of an application developer reading this doc? Done http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/scan_token-internal.cc@253 PS9, Line 253: LOG(FATAL) << "Ignoring snapshot timestamp since not in " : "READ_AT_SNAPSH > should we validate this on the config? i.e. don't allow to pass a snapshot Done http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/scanner-internal.cc@330 PS9, Line 330: if (configuration_.has_snapshot_timestamp()) { : LOG(FATAL) << "Ignoring snapshot timestamp since " : "not in READ_AT_SNAPSHOT mode."; : } > same comment I did elsewhere Done -- To view, visit http://gerrit.cloudera.org:8080/8823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34214245a78aed172a28fbdb395ff5bccd0fc0e1 Gerrit-Change-Number: 8823 Gerrit-PatchSet: 10 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Wed, 21 Feb 2018 05:24:02 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add c++ client support for READ YOUR WRITES mode
Hello Alex Rodoni, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8823 to look at the new patch set (#10). Change subject: KUDU-1704: add c++ client support for READ_YOUR_WRITES mode .. KUDU-1704: add c++ client support for READ_YOUR_WRITES mode Change-Id: I34214245a78aed172a28fbdb395ff5bccd0fc0e1 --- M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/scan_configuration.cc M src/kudu/client/scan_configuration.h M src/kudu/client/scan_token-internal.cc M src/kudu/client/scan_token-test.cc M src/kudu/client/scanner-internal.cc M src/kudu/integration-tests/consistency-itest.cc M src/kudu/integration-tests/delete_table-itest.cc 10 files changed, 302 insertions(+), 61 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/8823/10 -- To view, visit http://gerrit.cloudera.org:8080/8823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I34214245a78aed172a28fbdb395ff5bccd0fc0e1 Gerrit-Change-Number: 8823 Gerrit-PatchSet: 10 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 18: (2 comments) http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163 PS16, Line 2163: > Thanks for the explanation. Let's add that explanation in the comment here, Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163 PS16, Line 2163: > Maybe we should just add that explanation to the docs? Updated. -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 18 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 21 Feb 2018 05:23:20 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8804 to look at the new patch set (#18). Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions if possible. READ_YOUR_WRITES mode is not repeatable. However, it allows client local read-your-writes/read-your-reads. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 6 files changed, 326 insertions(+), 98 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/18 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 18 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9363 ) Change subject: KUDU-721: Support range partitions on decimal columns .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc@1639 PS1, Line 1639: schema; > nit: I don't think we use identifiers like 'x_' for local variables; would Ah, right. I copied this test from the UnixTimeMicros one above. Will update both. http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc@1643 PS1, Line 1643: ASSERT_O > why not just ASSERT_OK() ? Done http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc@1645 PS1, Line 1645: unique_ptr lower_bound(schema.NewRow()); : ASSERT_OK(lower_bound->SetUnscaledDecimal("key", -1)); : unique_ptr upper_bound(schema.NewRow()); : ASSERT_OK(upper_bound->SetUnscaledDecimal("key", 99)); > I think it would be nice to add a 'negative' test case (maybe, in some diff Done http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/common/partition.cc@1043 PS1, Line 1043: const ColumnS > const reference? Done -- To view, visit http://gerrit.cloudera.org:8080/9363 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917 Gerrit-Change-Number: 9363 Gerrit-PatchSet: 2 Gerrit-Owner: Grant HenkeGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 21 Feb 2018 05:01:38 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Hello Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9363 to look at the new patch set (#2). Change subject: KUDU-721: Support range partitions on decimal columns .. KUDU-721: Support range partitions on decimal columns Add the DataType handling to enable range partitions on decimal columns and a test that fails without the changes. Also adds range partition tests for edge cases where: * The upper bound is less than than the lower bound * The upper bound is equal to the lower bound * The bounds use MIN/MAX values Change-Id: I419d442dd233c73166c6e391907c8443ebecc917 --- M src/kudu/client/client-test.cc M src/kudu/common/partition.cc M src/kudu/util/decimal_util-test.cc M src/kudu/util/decimal_util.cc M src/kudu/util/decimal_util.h 5 files changed, 155 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/9363/2 -- To view, visit http://gerrit.cloudera.org:8080/9363 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917 Gerrit-Change-Number: 9363 Gerrit-PatchSet: 2 Gerrit-Owner: Grant HenkeGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2291 (part 3): use futex to speed up stack collection
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9254 ) Change subject: KUDU-2291 (part 3): use futex to speed up stack collection .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9254 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icb772f0607275ec340c7f38be05347fc14820cbd Gerrit-Change-Number: 9254 Gerrit-PatchSet: 6 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 21 Feb 2018 03:07:21 + Gerrit-HasComments: No
[kudu-CR] KUDU-2291 (part 5): allow collecting stack traces asynchronously
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9318 ) Change subject: KUDU-2291 (part 5): allow collecting stack traces asynchronously .. Patch Set 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.h File src/kudu/util/debug-util.h: http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.h@208 PS4, Line 208: Trigger TriggerAsync http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc File src/kudu/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@141 PS4, Line 141: void Signal() { add: // Wakes all waiters, notifying them of completion. http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@151 PS4, Line 151: bool WaitUntil(MonoTime deadline) { add: // Returns false if reached deadline before completion, true otherwise. http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@216 PS4, Line 216: maybe nit: capitalize http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@219 PS4, Line 219: ANNOTATE_HAPPENS_AFTER(sig); why is this needed? http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@230 PS4, Line 230: 2 nit: /*skip_frames=*/ http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@312 PS4, Line 312: } nit: move up one line http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@395 PS4, Line 395: SYS_rt_tgsigqueueinfo Did you look into using pthread_sigqueue() ? I suppose a minor benefit of this approach is that we can use the native tid? http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@396 PS4, Line 396: ) , ErrnoToString(errno), errno) ? http://gerrit.cloudera.org:8080/#/c/9318/4/src/kudu/util/debug-util.cc@427 PS4, Line 427: Status StackTraceCollector::TriggerAsync(int64_t tid_, StackTrace* stack) { missing void StackTraceCollector::RevokeSigData() for non-linux http://gerrit.cloudera.org:8080/#/c/9318/5/src/kudu/util/debug-util.cc File src/kudu/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/9318/5/src/kudu/util/debug-util.cc@213 PS5, Line 213: sig nit: name this sig_data for consistency when tracing both sides of the communication logic. http://gerrit.cloudera.org:8080/#/c/9318/5/src/kudu/util/debug-util.cc@350 PS5, Line 350: DLOG(WARNING) << "Leaking SignalData structure " << sig_data_ << " after lost signal " I think we could at least make the signalled thread try to delete the sig_data structure if the caller has given up. I haven't tested this but I think we could use something like: - if (!sig->queued_to_tid.compare_exchange_strong(my_tid, SignalData::kDumpInProgress)) { + int64_t old_val = sig->queued_to_tid.exchange(SignalData::kDumpInProgress); + if (old_val != my_tid) { +delete sig; return; } -- To view, visit http://gerrit.cloudera.org:8080/9318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie4d7789bb1272db033df4685f46eaf8483ec7be1 Gerrit-Change-Number: 9318 Gerrit-PatchSet: 5 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 21 Feb 2018 03:06:10 + Gerrit-HasComments: Yes
[kudu-CR] WIP: KUDU-2297 (part 5): dump stacks into diagnostics log on service queue overflow
Todd Lipcon has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9375 Change subject: WIP: KUDU-2297 (part 5): dump stacks into diagnostics log on service queue overflow .. WIP: KUDU-2297 (part 5): dump stacks into diagnostics log on service queue overflow In the past we've had several bugs and performance issues which manifest themselves as service queue overflows. It would be easier to pinpoint the root cause (eg slow IO vs lock contention) if we had a process-wide stack snapshot at the time of the overflow. This patch does some plumbing to trigger such a stack trace into the diagnostics log when a service queue overflow occurs. The logging is throttled to once every 5 seconds so that we don't contribute too much to the load itself. The plumbing is a bit ugly since the diagnostics log is in the server/ module whereas the overflows surface in the rpc/ module. Having rpc/ call back into server/ would be a cyclic dependency, so instead the rpc/ module exposes a std::function<> style callback for the server to hook into. WIP because I'd like to add some kind of test for this, though I tested manually and it appears to work. Change-Id: I62019c71121d7ca4037cab86a7c2ea048a261ad8 --- M src/kudu/rpc/service_pool.cc M src/kudu/rpc/service_pool.h M src/kudu/server/diagnostics_log.cc M src/kudu/server/diagnostics_log.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/server/server_base.h 8 files changed, 96 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/9375/1 -- To view, visit http://gerrit.cloudera.org:8080/9375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I62019c71121d7ca4037cab86a7c2ea048a261ad8 Gerrit-Change-Number: 9375 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon
[kudu-CR] KUDU-2297 (part 1): refactor metrics log out of ServerBase
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9326 to look at the new patch set (#5). Change subject: KUDU-2297 (part 1): refactor metrics log out of ServerBase .. KUDU-2297 (part 1): refactor metrics log out of ServerBase This is preparing for putting more information into the metrics log instead of just metrics snapshots. The logging code will get more complex as it gains features, so this makes a new class for it. This is slightly incompatible since I also changed the name on disk to 'diagnostics' instead of 'metrics' and updated the documentation, but I am not aware of people using this in the wild. So long as we release-note it, I think it's reasonable to expect any sophisticated operators to adjust their scripting accordingly. Change-Id: I1f7bc7335b1f4c2a5c62fc5fe672289d5027a10d --- M docs/administration.adoc M src/kudu/server/CMakeLists.txt A src/kudu/server/diagnostics_log.cc A src/kudu/server/diagnostics_log.h M src/kudu/server/server_base.cc M src/kudu/server/server_base.h 6 files changed, 241 insertions(+), 73 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/9326/5 -- To view, visit http://gerrit.cloudera.org:8080/9326 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1f7bc7335b1f4c2a5c62fc5fe672289d5027a10d Gerrit-Change-Number: 9326 Gerrit-PatchSet: 5 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2291 (part 3): use futex to speed up stack collection
Hello Will Berkeley, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9254 to look at the new patch set (#6). Change subject: KUDU-2291 (part 3): use futex to speed up stack collection .. KUDU-2291 (part 3): use futex to speed up stack collection Previously the thread which requests a stack trace would poll on an atomic variable waiting for the stack to be collected. Of course spinning is too CPU-hungry so it would actually sleep for 10ms in each iteration. This limited the performance of stack collection to about 100/second, which is problematic for the /stacks web page on a server that may have thousands of threads. This switches to using the futex system call to allow the dumper thread to sleep and the dumpee thread to wake it back up when the results are ready. Why do we use the low level futex and not something like a condition variable or CountdownLatch? The issue is that the dumpee thread is running in a signal handler context, and most things are not safe to do in that context. Particularly, mutexes may block, which is very naughty, and condition variables and latches are currently implemented on top of mutexes. pthread semaphores are another option, but those don't support proper timeout semantics, which is a feature we use in this use case. Whether the futex syscall itself is async-signal-safe is up for some debate. Clearly the "wait" mode is not, since it may block, but is the "wake" mode that we use here allowed? Technically I don't think so. In practice, the pthread sem_post() API _is_ documented to be async-signal-safe and I verified that in libc source it's implemented using futex to wake the waiter. So, if we are doing something illegal, at least we're doing the same illegal thing as libc, so we're very unlikely to ever break. Obviously futex is not portable to macOS, but this code was already Linux-specific so we haven't lost much. This speeds up the benchmark for stack trace collection (without symbolization) by 585x: Before: I0207 19:33:20.960477 30799 debug-util-test.cc:175] Throughput: 99 dumps/second (symbolize=0) I0207 19:33:21.963946 30799 debug-util-test.cc:175] Throughput: 84 dumps/second (symbolize=1) After: I0207 19:38:32.505213 32039 debug-util-test.cc:175] Throughput: 57916 dumps/second (symbolize=0) I0207 19:38:33.506417 32039 debug-util-test.cc:175] Throughput: 595 dumps/second (symbolize=1) Change-Id: Icb772f0607275ec340c7f38be05347fc14820cbd --- M src/kudu/util/debug-util.cc 1 file changed, 70 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/54/9254/6 -- To view, visit http://gerrit.cloudera.org:8080/9254 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icb772f0607275ec340c7f38be05347fc14820cbd Gerrit-Change-Number: 9254 Gerrit-PatchSet: 6 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2297 (part 4): periodically dump stacks to diagnostics log
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9330 to look at the new patch set (#6). Change subject: KUDU-2297 (part 4): periodically dump stacks to diagnostics log .. KUDU-2297 (part 4): periodically dump stacks to diagnostics log This modifies the diagnostics log to periodically dump stack traces. This is slightly complicated by the fact that symbolized stack traces can be relatively large. So, we separate the logging of symbols and stack traces. When an address first appears in a log file, it is logged as part of a symbol line. Later logs of the same address do not need to re-log the symbol. With this, a typical stack trace dump of an idle tserver is about 4KB pre-compression, and a 'symbols' dump is about 6KB. So logging stacks reasonably often should not use much disk space or IO. Currently this is enabled on the same interval as the metrics log, but only if a new experimental flag --diagnostics-log-stack-traces is enabled. I'm planning to move it to a different flag in a later commit, but wanted to keep this one simple and incremental. Change-Id: Ic32abf2c48ac6a5f3c384e58838b34671bcaf147 --- M src/kudu/server/diagnostics_log.cc M src/kudu/server/diagnostics_log.h M src/kudu/util/debug-util.cc M src/kudu/util/debug-util.h M src/kudu/util/rolling_log-test.cc M src/kudu/util/rolling_log.cc M src/kudu/util/rolling_log.h 7 files changed, 227 insertions(+), 41 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/9330/6 -- To view, visit http://gerrit.cloudera.org:8080/9330 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic32abf2c48ac6a5f3c384e58838b34671bcaf147 Gerrit-Change-Number: 9330 Gerrit-PatchSet: 6 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] [catalog manager] check re-replication scheme upon table creation
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9321 ) Change subject: [catalog_manager] check re-replication scheme upon table creation .. [catalog_manager] check re-replication scheme upon table creation Output actionable warning message upon creation of a new table when the number of live tablet servers is not enough to re-replicate tablet replicas in case of a failure. Since the 3-4-3 replication scheme is now enabled by default, this might be useful in case if running a cluster with just N tablet servers when newly created tables have replication factor of N (e.g., imagine running with just 3 tablet servers total and creating tables with the default replication factor of 3). This is a follow-up for c40e0587bf6a6aa55e5bd72dd2dd9356b1507f2e. Change-Id: I5a59451ad876d9864a90607187a05f8526cd8cbf Reviewed-on: http://gerrit.cloudera.org:8080/9321 Tested-by: Kudu Jenkins Reviewed-by: Mike Percy--- M src/kudu/master/catalog_manager.cc 1 file changed, 36 insertions(+), 16 deletions(-) Approvals: Kudu Jenkins: Verified Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/9321 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I5a59451ad876d9864a90607187a05f8526cd8cbf Gerrit-Change-Number: 9321 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] WIP KUDU-2319 follower masters cannot verify tokens
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9373 to look at the new patch set (#2). Change subject: WIP KUDU-2319 follower masters cannot verify tokens .. WIP KUDU-2319 follower masters cannot verify tokens This small test explains that follower masters still cannot accept authn tokens for verification: they don't have the public parts of TSKs generated by the leader master. WIP: it's necessary to fix the issue -- make follower masters to obtain the public parts of currently avaialable TSK keys for TokenVerifier WIP: after fixing the issue, it's necessary to enable currently disabled FollowerMastersHaveTokenVerificationKeys test. Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7 --- M src/kudu/integration-tests/security-itest.cc M src/kudu/integration-tests/security-master-certificates-itest.cc 2 files changed, 76 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/9373/2 -- To view, visit http://gerrit.cloudera.org:8080/9373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7 Gerrit-Change-Number: 9373 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2274 [itest] stress test for replica replacement
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9255 ) Change subject: KUDU-2274 [itest] stress test for replica replacement .. KUDU-2274 [itest] stress test for replica replacement This is a stress test for the automatic replica replacement in Kudu. Parameters of the test are configurable via run-time gflags, so it's possible to run it in a 'standalone' mode, targeting it to be a sort of an endurance test. This test reproduces the race described in KUDU-2274: it triggers DCHECK() assertions added in 194fd8b169f29aafbd78a47709ac51d2e8354a1a before the relevant fixes for KUDU-2274 were checked in. Change-Id: I036a882f7e9132a5c26013227c50c0699b59ed6e Reviewed-on: http://gerrit.cloudera.org:8080/9255 Tested-by: Kudu Jenkins Reviewed-by: Mike Percy--- M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/cluster_verifier.cc M src/kudu/integration-tests/cluster_verifier.h A src/kudu/integration-tests/raft_consensus_stress-itest.cc 4 files changed, 305 insertions(+), 4 deletions(-) Approvals: Kudu Jenkins: Verified Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/9255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I036a882f7e9132a5c26013227c50c0699b59ed6e Gerrit-Change-Number: 9255 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2259: add real user to AuthenticationCredentialsPB
Hello Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/9374 to review the following change. Change subject: KUDU-2259: add real user to AuthenticationCredentialsPB .. KUDU-2259: add real user to AuthenticationCredentialsPB WIP: the Java client needs a similar change This commit adds the 'real user' to the authn credentials token, which is used when negotiating connections with SASL PLAIN authentication. This is useful when scan tokens are being sent to remote tasks, it's not possible to authenticate with a signed authn token to the remote server[1], coarse-grained ACLs have been set, and the 'planner' and 'executor' processes are being run with different users. [1]: this most often occurs because the remote server has encryption disabled. Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45 --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.proto M src/kudu/client/master_rpc.cc M src/kudu/client/master_rpc.h M src/kudu/client/meta_cache.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/rpc/connection_id.cc M src/kudu/rpc/server_negotiation.cc M src/kudu/rpc/user_credentials.cc M src/kudu/rpc/user_credentials.h M src/kudu/util/user.cc 14 files changed, 102 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/9374/1 -- To view, visit http://gerrit.cloudera.org:8080/9374 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I5d2d901d42501ecfc0f6372f68cf7335eb188b45 Gerrit-Change-Number: 9374 Gerrit-PatchSet: 1 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Todd Lipcon
[kudu-CR] WIP follower masters cannot verify tokens
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9373 Change subject: WIP follower masters cannot verify tokens .. WIP follower masters cannot verify tokens This small test explains that follower masters still cannot accept authn tokens for verification: they don't have the public parts of TSKs generated by the leader master. WIP: it's necessary to fix the issue -- make follower masters to obtain the public parts of currently avaialable TSK keys for TokenVerifier Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7 --- M src/kudu/integration-tests/security-itest.cc M src/kudu/integration-tests/security-master-certificates-itest.cc 2 files changed, 64 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/9373/1 -- To view, visit http://gerrit.cloudera.org:8080/9373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7 Gerrit-Change-Number: 9373 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] gutil: properly hook up ANNOTATE HAPPENS BEFORE/AFTER
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9325 ) Change subject: gutil: properly hook up ANNOTATE_HAPPENS_BEFORE/AFTER .. gutil: properly hook up ANNOTATE_HAPPENS_BEFORE/AFTER This updates dynamic_annotations to hook up ANNOTATE_HAPPENS_BEFORE and ANNOTATE_HAPPENS_AFTER to the appropriate annotations in the TSAN runtime library. According to [1] (committed in 2011) the functions have been implemented in TSAN for quite some time, and it was an error that we weren't using them. The previous implementation of the function called some condition-variable annotations in the TSAN runtime library instead. Those functions actually turn out to be implemented as no-ops. So, I looked for existing call sites to ANNOTATE_HAPPENS_BEFORE and AFTER and removed them. This cleaned up atomic_refcount pretty substantially. Again I found a matching change[2] in Chromium, from which this code is derived. [1] https://codereview.chromium.org/6982022 [2] https://codereview.chromium.org/580813002 Change-Id: Ida27aff6b9771c0009fba5e31ec7a0c7c53caa59 Reviewed-on: http://gerrit.cloudera.org:8080/9325 Tested-by: Kudu Jenkins Reviewed-by: Mike Percy--- M src/kudu/gutil/atomic_refcount.h M src/kudu/gutil/dynamic_annotations.c M src/kudu/gutil/dynamic_annotations.h M src/kudu/gutil/once.cc M src/kudu/gutil/once.h 5 files changed, 22 insertions(+), 59 deletions(-) Approvals: Kudu Jenkins: Verified Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/9325 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ida27aff6b9771c0009fba5e31ec7a0c7c53caa59 Gerrit-Change-Number: 9325 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2274 [itest] stress test for replica replacement
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9255 ) Change subject: KUDU-2274 [itest] stress test for replica replacement .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I036a882f7e9132a5c26013227c50c0699b59ed6e Gerrit-Change-Number: 9255 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 21 Feb 2018 01:37:02 + Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 17: (1 comment) http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163 PS16, Line 2163: (propagation ts + 1) should not > Maybe we should just add that explanation to the docs? Thanks for the explanation. Let's add that explanation in the comment here, maybe something like: // fall into that category because calling clock->Update(propagated_timestamp) verifies that propagated_timestamp + FLAGS_max_clock_sync_error_usec is not too far into the future. -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 17 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 21 Feb 2018 01:36:01 + Gerrit-HasComments: Yes
[kudu-CR] gutil: properly hook up ANNOTATE HAPPENS BEFORE/AFTER
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9325 ) Change subject: gutil: properly hook up ANNOTATE_HAPPENS_BEFORE/AFTER .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9325 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ida27aff6b9771c0009fba5e31ec7a0c7c53caa59 Gerrit-Change-Number: 9325 Gerrit-PatchSet: 3 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 21 Feb 2018 01:22:02 + Gerrit-HasComments: No
[kudu-CR] KUDU-2291 (part 4): avoid potential for deadlocks in stack trace collection
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9262 ) Change subject: KUDU-2291 (part 4): avoid potential for deadlocks in stack trace collection .. Patch Set 5: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/9262/5/src/kudu/util/debug/unwind_safeness.cc File src/kudu/util/debug/unwind_safeness.cc: http://gerrit.cloudera.org:8080/#/c/9262/5/src/kudu/util/debug/unwind_safeness.cc@23 PS5, Line 23: // Based on public domain code by Aliaksey Kandratsenka at Do we need to add this to LICENSE? -- To view, visit http://gerrit.cloudera.org:8080/9262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I414718a26c4810de86a10397bb9803dc5e386d1a Gerrit-Change-Number: 9262 Gerrit-PatchSet: 5 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 21 Feb 2018 01:17:58 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2291 (part 4): avoid potential for deadlocks in stack trace collection
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9262 ) Change subject: KUDU-2291 (part 4): avoid potential for deadlocks in stack trace collection .. Patch Set 5: (2 comments) Nice. How the heck did you even stumble upon this workaround? http://gerrit.cloudera.org:8080/#/c/9262/5/src/kudu/util/debug/unwind_safeness.cc File src/kudu/util/debug/unwind_safeness.cc: http://gerrit.cloudera.org:8080/#/c/9262/5/src/kudu/util/debug/unwind_safeness.cc@53 PS5, Line 53: libdl nit: period at end http://gerrit.cloudera.org:8080/#/c/9262/5/src/kudu/util/debug/unwind_safeness.cc@63 PS5, Line 63: * nit: any reason to move the asterisks around from their usual place in these signatures? -- To view, visit http://gerrit.cloudera.org:8080/9262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I414718a26c4810de86a10397bb9803dc5e386d1a Gerrit-Change-Number: 9262 Gerrit-PatchSet: 5 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 21 Feb 2018 01:12:49 + Gerrit-HasComments: Yes
[kudu-CR] rowset metadata: cache min/max encoded keys
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9372 Change subject: rowset_metadata: cache min/max encoded keys .. rowset_metadata: cache min/max encoded keys This patch adds a new flag rowset_metadata_store_keys that, when true, will indicate that Kudu should duplicate diskrowset min/max keys into the rowset metadata. Doing so allows Kudu to read the keys from tablet metadata and bootstrap tablets without having to fully open the CFileReaders for the key columns of each rowset. A small test is added to tablet_server-test that ensures we don't read any extraneous bytes when starting up the tablet server if we're reading keys from the rowset metadata. Change-Id: I37d6f7160e3a7188753684e063963110f70e9b8d --- M src/kudu/tablet/cfile_set.cc M src/kudu/tablet/cfile_set.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/metadata.proto M src/kudu/tablet/rowset_metadata.cc M src/kudu/tablet/rowset_metadata.h M src/kudu/tserver/tablet_server-test.cc 7 files changed, 121 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/9372/1 -- To view, visit http://gerrit.cloudera.org:8080/9372 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I37d6f7160e3a7188753684e063963110f70e9b8d Gerrit-Change-Number: 9372 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] docs: improvements to transaction semantics
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/9235 ) Change subject: docs: improvements to transaction semantics .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/9235/1/docs/transaction_semantics.adoc File docs/transaction_semantics.adoc: http://gerrit.cloudera.org:8080/#/c/9235/1/docs/transaction_semantics.adoc@59 PS1, Line 59: with different levels of consistency or correctness guarantees. Scans can perform time-travel reads, i.e. nit: is this over the col limit? (seems like it, but i might be wrong as I forget if we have one for docs or what it is) http://gerrit.cloudera.org:8080/#/c/9235/1/docs/transaction_semantics.adoc@181 PS1, Line 181: `READ_LATEST`:: This is the default read mode. The server takes a snapshot of : the MVCC state and proceeds with the read immediately. Reads in this mode only yield : 'Read Committed' isolation. are you leaving the new mode out on purpose? http://gerrit.cloudera.org:8080/#/c/9235/1/docs/transaction_semantics.adoc@205 PS1, Line 205: Note don't we have a special "note" marker, or am I mistaken? http://gerrit.cloudera.org:8080/#/c/9235/1/docs/transaction_semantics.adoc@222 PS1, Line 222: Writes ahah, well caught :) -- To view, visit http://gerrit.cloudera.org:8080/9235 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I23a2751923a4214f52e6cdb233f7e3aeee207da2 Gerrit-Change-Number: 9235 Gerrit-PatchSet: 1 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 21 Feb 2018 01:00:06 + Gerrit-HasComments: Yes
[kudu-CR] WIP [tests] scenario to repro off-by-one error in TestWorkload
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/9277 ) Change subject: WIP [tests] scenario to repro off-by-one error in TestWorkload .. Patch Set 1: I briefly tried this with my WIP implementation of leader leases and the problem seemed to have gone away. While not definitive this is encouraging evidence that the problem indeed stems from the lack of leader leases. Conceptually this seems plausible too. Scenario: - Two replicas (a,b) dispute leadership of tablet A, i.e. both think they're leaders, but "b" is the actual most recent leader. - Client writes the last row to replica "b". - Client then performs a scan at snapshot for all rows on replica "a", sending the timestamp of the write. - Replica "a" thinking it's leader simply considers the current timestamp "safe" and executes a (non-repeatable) scan that doesn't include the last row written to b. Possible follow-up steps: - Retry the scan. If the problem goes away, then it's more likely that the problem stems from the lack of leader leases. - Even better, hack it so that we retry the scan _at the same snapshot timestamp_. - Add logging events (test only) that register the timestamps/safe time/chosen replica on the server side (hopefully showing the smoking gun). - Implement leader leader and put this test on top :) -- To view, visit http://gerrit.cloudera.org:8080/9277 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I60666b8b05dce8dd13fcdee6408c0930915ba0c1 Gerrit-Change-Number: 9277 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 21 Feb 2018 00:54:04 + Gerrit-HasComments: No
[kudu-CR] KUDU-2291 (part 3): use futex to speed up stack collection
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9254 ) Change subject: KUDU-2291 (part 3): use futex to speed up stack collection .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/9254/5/src/kudu/util/debug-util.cc File src/kudu/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/9254/5/src/kudu/util/debug-util.cc@144 PS5, Line 144: 0 > how about: nullptr /* ignored */ Done http://gerrit.cloudera.org:8080/#/c/9254/5/src/kudu/util/debug-util.cc@197 PS5, Line 197: spins waiting > s/spins waiting/waits/ Done -- To view, visit http://gerrit.cloudera.org:8080/9254 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icb772f0607275ec340c7f38be05347fc14820cbd Gerrit-Change-Number: 9254 Gerrit-PatchSet: 5 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 21 Feb 2018 00:49:59 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add java client support for READ YOUR WRITES mode
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8847 ) Change subject: KUDU-1704: add java client support for READ_YOUR_WRITES mode .. Patch Set 4: I'm slacking on this review a bit until we have something solid on the c++ side (which we seem close to now). it's easier to review by analogy :) -- To view, visit http://gerrit.cloudera.org:8080/8847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6239521c022147257859e399f55c6f3f945af465 Gerrit-Change-Number: 8847 Gerrit-PatchSet: 4 Gerrit-Owner: Hao HaoGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 21 Feb 2018 00:44:02 + Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 16: (1 comment) http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163 PS16, Line 2163: (propagation ts + 1) should not > As we update the clock based on propagated timestamp at L2140, if (propagat Maybe we should just add that explanation to the docs? Also: obvious while we have these things in context, but likely worth mentioning for the documentation value is that "clean" timestamp is, by definition in the past. Suggestion, something like: There is no need to validate if the chosen timestamp is too far in the future, as : - MVCC's 'clean' ts is by definition in the past (it's maximally bounded by safe time). - "propagated" ts was used to update the clock above and the update would have returned an error if the the timestamp was too far in the future. -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 16 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Wed, 21 Feb 2018 00:42:13 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2291 (part 3): use futex to speed up stack collection
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9254 ) Change subject: KUDU-2291 (part 3): use futex to speed up stack collection .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/9254/5/src/kudu/util/debug-util.cc File src/kudu/util/debug-util.cc: http://gerrit.cloudera.org:8080/#/c/9254/5/src/kudu/util/debug-util.cc@144 PS5, Line 144: 0 how about: nullptr /* ignored */ http://gerrit.cloudera.org:8080/#/c/9254/5/src/kudu/util/debug-util.cc@197 PS5, Line 197: spins waiting s/spins waiting/waits/ -- To view, visit http://gerrit.cloudera.org:8080/9254 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icb772f0607275ec340c7f38be05347fc14820cbd Gerrit-Change-Number: 9254 Gerrit-PatchSet: 5 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 21 Feb 2018 00:37:42 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2297 (part 4): periodically dump stacks to diagnostics log
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9330 to look at the new patch set (#5). Change subject: KUDU-2297 (part 4): periodically dump stacks to diagnostics log .. KUDU-2297 (part 4): periodically dump stacks to diagnostics log This modifies the diagnostics log to periodically dump stack traces. This is slightly complicated by the fact that symbolized stack traces can be relatively large. So, we separate the logging of symbols and stack traces. When an address first appears in a log file, it is logged as part of a symbol line. Later logs of the same address do not need to re-log the symbol. With this, a typical stack trace dump of an idle tserver is about 4KB pre-compression, and a 'symbols' dump is about 6KB. So logging stacks reasonably often should not use much disk space or IO. Currently this is enabled on the same interval as the metrics log, but only if a new experimental flag --diagnostics-log-stack-traces is enabled. I'm planning to move it to a different flag in a later commit, but wanted to keep this one simple and incremental. Change-Id: Ic32abf2c48ac6a5f3c384e58838b34671bcaf147 --- M src/kudu/server/diagnostics_log.cc M src/kudu/server/diagnostics_log.h M src/kudu/util/debug-util.cc M src/kudu/util/debug-util.h M src/kudu/util/rolling_log-test.cc M src/kudu/util/rolling_log.cc M src/kudu/util/rolling_log.h 7 files changed, 227 insertions(+), 41 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/9330/5 -- To view, visit http://gerrit.cloudera.org:8080/9330 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic32abf2c48ac6a5f3c384e58838b34671bcaf147 Gerrit-Change-Number: 9330 Gerrit-PatchSet: 5 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2291 (part 6) stacks: use libunwind directly for stack tracing
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9319 to look at the new patch set (#5). Change subject: KUDU-2291 (part 6) stacks: use libunwind directly for stack tracing .. KUDU-2291 (part 6) stacks: use libunwind directly for stack tracing Previously we used glog's wrapper around libunwind for stack tracing. However that has a deficiency that it assumes that, process wide, only one thread can be inside libunwind at a time[1] It appears that this is left over from some very old versions of libunwind, or was already unnecessarily conservative. libunwind is meant to be thread safe, and we have tests that will trigger if it is not. This just extracts the function body of the glog function we were using and does the same work manually. Without this fix, the "collect from all the threads at the same time" code path resulted in most of the threads collecting an empty trace since they tried to call libunwind at the same time. [1] https://github.com/google/glog/issues/298 Change-Id: I3a53e55d7c4e7ee50bcac5b1e81267df56383634 --- M build-support/iwyu/iwyu.sh A build-support/iwyu/mappings/libunwind.imp M src/kudu/util/debug-util-test.cc M src/kudu/util/debug-util.cc M src/kudu/util/debug-util.h 5 files changed, 81 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/9319/5 -- To view, visit http://gerrit.cloudera.org:8080/9319 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3a53e55d7c4e7ee50bcac5b1e81267df56383634 Gerrit-Change-Number: 9319 Gerrit-PatchSet: 5 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2297 (part 3): refactor process-wide stack collection out of /stacks
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9329 to look at the new patch set (#4). Change subject: KUDU-2297 (part 3): refactor process-wide stack collection out of /stacks .. KUDU-2297 (part 3): refactor process-wide stack collection out of /stacks Previously a bunch of logic to collect all the stacks from the process was in the /stacks path handler. This logic is relatively generic and shouldn't be intermingled with the formatting code. In particular I'd like to use it in the diagnostics log, where a different output format is desirable. Change-Id: Ibb7c6edd31254f3d7e0cbef1eaf575bde3570df6 --- M src/kudu/server/default_path_handlers.cc M src/kudu/server/diagnostics_log.cc M src/kudu/util/debug-util-test.cc M src/kudu/util/debug-util.cc M src/kudu/util/debug-util.h 5 files changed, 190 insertions(+), 112 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/9329/4 -- To view, visit http://gerrit.cloudera.org:8080/9329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibb7c6edd31254f3d7e0cbef1eaf575bde3570df6 Gerrit-Change-Number: 9329 Gerrit-PatchSet: 4 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2297 (part 2). Convert diagnostics log to a format closer to glog
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9327 to look at the new patch set (#4). Change subject: KUDU-2297 (part 2). Convert diagnostics log to a format closer to glog .. KUDU-2297 (part 2). Convert diagnostics log to a format closer to glog Previously the metrics/diagnostics log used a custom log format designed to be readable by scripts. The only timestamps were unix microtimes, which are convenient for computers but not for people. In order to make the log more easily greppable, this patch converts it over to use a glog-compatible prefix on each log line. The aim is that, if an admin sees some issues at 10/23 15:03 they can simply grep the diagnostics log for '1021 15:0[0-6]' rather than having to go figure out the appropriate range of numeric timestamps. This patch also updates the docs accordingly. Change-Id: I10d93c84f8c738c26850ae3d1ae6afbb5519c7ad --- M docs/administration.adoc M src/kudu/server/diagnostics_log.cc M src/kudu/server/diagnostics_log.h M src/kudu/util/logging.cc M src/kudu/util/logging.h M src/kudu/util/trace.cc 6 files changed, 47 insertions(+), 29 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/9327/4 -- To view, visit http://gerrit.cloudera.org:8080/9327 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I10d93c84f8c738c26850ae3d1ae6afbb5519c7ad Gerrit-Change-Number: 9327 Gerrit-PatchSet: 4 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2297 (part 1): refactor metrics log out of ServerBase
Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9326 to look at the new patch set (#4). Change subject: KUDU-2297 (part 1): refactor metrics log out of ServerBase .. KUDU-2297 (part 1): refactor metrics log out of ServerBase This is preparing for putting more information into the metrics log instead of just metrics snapshots. The logging code will get more complex as it gains features, so this makes a new class for it. This is slightly incompatible since I also changed the name on disk to 'diagnostics' instead of 'metrics' and updated the documentation, but I am not aware of people using this in the wild. So long as we release-note it, I think it's reasonable to expect any sophisticated operators to adjust their scripting accordingly. Change-Id: I1f7bc7335b1f4c2a5c62fc5fe672289d5027a10d --- M docs/administration.adoc M src/kudu/server/CMakeLists.txt A src/kudu/server/diagnostics_log.cc A src/kudu/server/diagnostics_log.h M src/kudu/server/server_base.cc M src/kudu/server/server_base.h 6 files changed, 240 insertions(+), 73 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/9326/4 -- To view, visit http://gerrit.cloudera.org:8080/9326 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1f7bc7335b1f4c2a5c62fc5fe672289d5027a10d Gerrit-Change-Number: 9326 Gerrit-PatchSet: 4 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] internal mini cluster: support Cluster/LogVerifier
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9137 ) Change subject: internal_mini_cluster: support Cluster/LogVerifier .. internal_mini_cluster: support Cluster/LogVerifier This patch introduces a MiniCluster-agnostic MiniClusterFsInspector. With this, the LogVerifier, and thus, the ClusterVerifier can support both External- and InternalMiniClusters. The LogVerifier would originally open a read-only FsManager (which in turn would open a BlockManager and a DataDirManager) per server and pass it to the LogReader to inspect the WALs. Instead, the LogVerifier now creates a MiniClusterFsInspector, which is more lightweight and defined per cluster. To the LogReader, it passes the WAL directory and an env, which is all the LogReader needed from the FsManager in the first place. To test, I updated a test case in ts_tablet_manager-itest to make use of the new ClusterVerifier with an internal cluster. CheckCluster() uses a LogVerifier, which in this case uses an MiniClusterFsInspector that operates on an InternalMiniCluster. Change-Id: I228a6e3ba1a42db4e243ffdc5116f0c60ee04a84 Reviewed-on: http://gerrit.cloudera.org:8080/9137 Tested-by: Kudu Jenkins Reviewed-by: Mike Percy--- M src/kudu/consensus/log-test.cc M src/kudu/consensus/log_reader.cc M src/kudu/consensus/log_reader.h M src/kudu/fs/fs_manager.h M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/client_failover-itest.cc M src/kudu/integration-tests/cluster_verifier.cc M src/kudu/integration-tests/create-table-itest.cc M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/external_mini_cluster-itest-base.cc M src/kudu/integration-tests/external_mini_cluster-itest-base.h M src/kudu/integration-tests/log_verifier.cc M src/kudu/integration-tests/log_verifier.h R src/kudu/integration-tests/mini_cluster_fs_inspector.cc R src/kudu/integration-tests/mini_cluster_fs_inspector.h M src/kudu/integration-tests/raft_config_change-itest.cc M src/kudu/integration-tests/raft_consensus-itest-base.cc M src/kudu/integration-tests/raft_consensus-itest.cc M src/kudu/integration-tests/raft_consensus_election-itest.cc M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/tablet_copy_client_session-itest.cc M src/kudu/integration-tests/tablet_replacement-itest.cc M src/kudu/integration-tests/tombstoned_voting-itest.cc M src/kudu/integration-tests/tombstoned_voting-stress-test.cc M src/kudu/integration-tests/ts_itest-base.cc M src/kudu/integration-tests/ts_itest-base.h M src/kudu/integration-tests/ts_recovery-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/kudu-ts-cli-test.cc 37 files changed, 313 insertions(+), 305 deletions(-) Approvals: Kudu Jenkins: Verified Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/9137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I228a6e3ba1a42db4e243ffdc5116f0c60ee04a84 Gerrit-Change-Number: 9137 Gerrit-PatchSet: 13 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2291 (part 2): Add a /stacks page
Mike Percy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9253 ) Change subject: KUDU-2291 (part 2): Add a /stacks page .. KUDU-2291 (part 2): Add a /stacks page This adds a simple /stacks web page which dumps the currently running threads in a plain text format. Since we often have a lot of idle threadpool threads sitting around in the "wait for work" state, the output collapses all threads with matching stacks and only displays the stack once, making it more suitable for human consumption. Example output from a local kudu-master: https://gist.github.com/b64739ee5fb146ea1953380f57b996c4 Longer term we may want to integrate stack-tracing capability into the /threadz view as well, but for now I left this as a low-level utility which doesn't access the thread manager, etc. I left a few TODOs for further enhancements, but I've already found this helpful for understanding some perf anomalies while playing with YCSB, so let's get it committed and improve as we go. Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b Reviewed-on: http://gerrit.cloudera.org:8080/9253 Tested-by: Todd LipconReviewed-by: Mike Percy --- M src/kudu/integration-tests/linked_list-test-util.h M src/kudu/server/default_path_handlers.cc M src/kudu/util/debug-util.cc 3 files changed, 106 insertions(+), 9 deletions(-) Approvals: Todd Lipcon: Verified Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/9253 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b Gerrit-Change-Number: 9253 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2291 (part 2): Add a /stacks page
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9253 ) Change subject: KUDU-2291 (part 2): Add a /stacks page .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9253 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b Gerrit-Change-Number: 9253 Gerrit-PatchSet: 5 Gerrit-Owner: Todd LipconGerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 21 Feb 2018 00:07:14 + Gerrit-HasComments: No
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/8/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/8/src/kudu/rpc/rpc-test.cc@28 PS8, Line 28: #include > warning: #includes are not sorted properly [llvm-include-order] Done -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 9 Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 23:56:34 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Tidy Bot, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#9). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 68 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/9 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 9 Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2274 [itest] stress test for replica replacement
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9255 ) Change subject: KUDU-2274 [itest] stress test for replica replacement .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/9255/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9255/5//COMMIT_MSG@11 PS5, Line 11: ar > are Done http://gerrit.cloudera.org:8080/#/c/9255/5/src/kudu/integration-tests/raft_consensus_stress-itest.cc File src/kudu/integration-tests/raft_consensus_stress-itest.cc: http://gerrit.cloudera.org:8080/#/c/9255/5/src/kudu/integration-tests/raft_consensus_stress-itest.cc@236 PS5, Line 236: > nit: why not iteration++ here? Because I don't want to count retry due to a ksck failure as an iteration: if RunKsck() fails, the logic starts another loop, but that case is not counted as 'iteration' in sense of this scenario. http://gerrit.cloudera.org:8080/#/c/9255/5/src/kudu/integration-tests/raft_consensus_stress-itest.cc@277 PS5, Line 277: if (ksck_failures_in_a_row > FLAGS_test_max_ksck_failures) { : // Suspecting a Raft consensus failure while running ksck with shorter : // timeout (see above). Run an extra round of ksck with the regular timeout : // to verify that replicas haven't really converged and, if so, just bail : // right at this point. : const auto& s = v.RunKsck(); : if (!s.ok()) { : FAIL() << Substitute("$0: tablet replicas haven't converged", s.ToString()); : } : } > isn't this the same as line 288 where we run v.CheckCluster() ? Yes, CheckCluster contains RunKsck(). However, it's wrapped into some extra logic for retries. Here I wanted to short-circuit and report an error right away if the ksck has already failed too many times in a row. -- To view, visit http://gerrit.cloudera.org:8080/9255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I036a882f7e9132a5c26013227c50c0699b59ed6e Gerrit-Change-Number: 9255 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 20 Feb 2018 23:54:32 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2274 [itest] stress test for replica replacement
Hello Tidy Bot, Mike Percy, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9255 to look at the new patch set (#6). Change subject: KUDU-2274 [itest] stress test for replica replacement .. KUDU-2274 [itest] stress test for replica replacement This is a stress test for the automatic replica replacement in Kudu. Parameters of the test are configurable via run-time gflags, so it's possible to run it in a 'standalone' mode, targeting it to be a sort of an endurance test. This test reproduces the race described in KUDU-2274: it triggers DCHECK() assertions added in 194fd8b169f29aafbd78a47709ac51d2e8354a1a before the relevant fixes for KUDU-2274 were checked in. Change-Id: I036a882f7e9132a5c26013227c50c0699b59ed6e --- M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/cluster_verifier.cc M src/kudu/integration-tests/cluster_verifier.h A src/kudu/integration-tests/raft_consensus_stress-itest.cc 4 files changed, 305 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/9255/6 -- To view, visit http://gerrit.cloudera.org:8080/9255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I036a882f7e9132a5c26013227c50c0699b59ed6e Gerrit-Change-Number: 9255 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 16: (10 comments) http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc@a1726 PS16, Line 1726: > What happened to this test? Maybe I'm missing something, but it seems this Since ScannerOpenWhenServerShutsDownParamTest should also cover the scenario of this test case. I removed it as it is duplicated. http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h File src/kudu/tserver/tablet_service.h: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@157 PS16, Line 157: TimeStamp > nit: in the code around, we have Timestamp as a type; maybe, name it consis Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@161 PS16, Line 161: PickAndVerifyTimeStamp > nit: ditto, PickAndVerifyTimestamp(...) ? Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@162 PS16, Line 162: tablet::TabletReplica* > Could it be const tablet::TabletReplica& ? Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@1757 PS16, Line 1757: case READ_YOUR_WRITES: > nit: per coding standard, add // fallthrough Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2042 PS16, Line 2042: case READ_AT_SNAPSHOT: > nit: per coding standard, add // fallthrough Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2117 PS16, Line 2117: . > s/./,/ Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118 PS16, Line 2118: initiated > s/initiated with/default-constructed to/ Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118 PS16, Line 2118: Since > s/Since/since/ Done http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163 PS16, Line 2163: (propagation ts + 1) should not > is that guaranteed by something? As we update the clock based on propagated timestamp at L2140, if (propagation ts + 1) is a timestamp too far in the future, which essentially means (propagation TS + 1 > propagation TS + FLAGS_max_clock_sync_error_usec). -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 16 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 20 Feb 2018 23:50:48 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8804 to look at the new patch set (#17). Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. KUDU-1704: add READ_YOUR_WRITES scan mode This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode, the server will pick a snapshot in the past, subject to the propagated timestamp bound, and perform a read. Moreover, the chosen snapshot timestamp is returned back to the client. The major difference between READ_AT_SNAPSHOT scan without a timestamp and READ_YOUR_WRITES scan is the latter will choose the newest timestamp within the staleness bound that allows execution of the reads without being blocked by the in-flight transactions if possible. READ_YOUR_WRITES mode is not repeatable. However, it allows client local read-your-writes/read-your-reads. Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 --- M src/kudu/common/common.proto M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h 6 files changed, 322 insertions(+), 98 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/17 -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 17 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/7/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/7/src/kudu/rpc/rpc-test.cc@494 PS7, Line 494: ASSERT_OK(client_messenger->DumpRunningRpcs(dump_req, _resp)); > Can you try looping this test with --stress-cpu-threads=$(nproc) to double I ran the test with --stress_cpu_threads=$(nrpoc) for about an hour (~800 iterations), and there were no failures. -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 8 Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 23:44:39 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2297 (part 1): refactor metrics log out of ServerBase
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9326 ) Change subject: KUDU-2297 (part 1): refactor metrics log out of ServerBase .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/9326/3/src/kudu/server/diagnostics_log.h File src/kudu/server/diagnostics_log.h: http://gerrit.cloudera.org:8080/#/c/9326/3/src/kudu/server/diagnostics_log.h@39 PS3, Line 39: DiagnosticsLog(std::string log_dir, > warning: function 'kudu::server::DiagnosticsLog::DiagnosticsLog' has a defi Done http://gerrit.cloudera.org:8080/#/c/9326/3/src/kudu/server/diagnostics_log.cc File src/kudu/server/diagnostics_log.cc: http://gerrit.cloudera.org:8080/#/c/9326/3/src/kudu/server/diagnostics_log.cc@104 PS3, Line 104: Unable to collect metrics to diagnostics log > Add a bit saying we'll retry in `kWaitBetweenFailures` seconds. Done http://gerrit.cloudera.org:8080/#/c/9326/3/src/kudu/server/diagnostics_log.cc@118 PS3, Line 118: this seems redundant with the "only include changed metrics" above > Referring to `opts.only_modified_in_or_after_epoch` below? Done -- To view, visit http://gerrit.cloudera.org:8080/9326 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f7bc7335b1f4c2a5c62fc5fe672289d5027a10d Gerrit-Change-Number: 9326 Gerrit-PatchSet: 3 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 20 Feb 2018 23:44:07 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#8). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 68 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/8 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 8 Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2274 [itest] stress test for replica replacement
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9255 ) Change subject: KUDU-2274 [itest] stress test for replica replacement .. Patch Set 5: Code-Review+1 (3 comments) http://gerrit.cloudera.org:8080/#/c/9255/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9255/5//COMMIT_MSG@11 PS5, Line 11: is are http://gerrit.cloudera.org:8080/#/c/9255/5/src/kudu/integration-tests/raft_consensus_stress-itest.cc File src/kudu/integration-tests/raft_consensus_stress-itest.cc: http://gerrit.cloudera.org:8080/#/c/9255/5/src/kudu/integration-tests/raft_consensus_stress-itest.cc@236 PS5, Line 236: nit: why not iteration++ here? http://gerrit.cloudera.org:8080/#/c/9255/5/src/kudu/integration-tests/raft_consensus_stress-itest.cc@277 PS5, Line 277: if (ksck_failures_in_a_row > FLAGS_test_max_ksck_failures) { : // Suspecting a Raft consensus failure while running ksck with shorter : // timeout (see above). Run an extra round of ksck with the regular timeout : // to verify that replicas haven't really converged and, if so, just bail : // right at this point. : const auto& s = v.RunKsck(); : if (!s.ok()) { : FAIL() << Substitute("$0: tablet replicas haven't converged", s.ToString()); : } : } isn't this the same as line 288 where we run v.CheckCluster() ? -- To view, visit http://gerrit.cloudera.org:8080/9255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I036a882f7e9132a5c26013227c50c0699b59ed6e Gerrit-Change-Number: 9255 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 20 Feb 2018 23:42:10 + Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] check re-replication scheme upon table creation
Hello Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9321 to look at the new patch set (#3). Change subject: [catalog_manager] check re-replication scheme upon table creation .. [catalog_manager] check re-replication scheme upon table creation Output actionable warning message upon creation of a new table when the number of live tablet servers is not enough to re-replicate tablet replicas in case of a failure. Since the 3-4-3 replication scheme is now enabled by default, this might be useful in case if running a cluster with just N tablet servers when newly created tables have replication factor of N (e.g., imagine running with just 3 tablet servers total and creating tables with the default replication factor of 3). This is a follow-up for c40e0587bf6a6aa55e5bd72dd2dd9356b1507f2e. Change-Id: I5a59451ad876d9864a90607187a05f8526cd8cbf --- M src/kudu/master/catalog_manager.cc 1 file changed, 36 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/9321/3 -- To view, visit http://gerrit.cloudera.org:8080/9321 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5a59451ad876d9864a90607187a05f8526cd8cbf Gerrit-Change-Number: 9321 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] [catalog manager] check re-replication scheme upon table creation
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9321 ) Change subject: [catalog_manager] check re-replication scheme upon table creation .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/9321/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/9321/1/src/kudu/master/catalog_manager.cc@1386 PS1, Line 1386: live > nit: s/alive/live/ Done http://gerrit.cloudera.org:8080/#/c/9321/1/src/kudu/master/catalog_manager.cc@1401 PS1, Line 1401: > add: (not recommended) Done -- To view, visit http://gerrit.cloudera.org:8080/9321 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5a59451ad876d9864a90607187a05f8526cd8cbf Gerrit-Change-Number: 9321 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 20 Feb 2018 23:13:11 + Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] check re-replication scheme upon table creation
Hello Mike Percy, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9321 to look at the new patch set (#2). Change subject: [catalog_manager] check re-replication scheme upon table creation .. [catalog_manager] check re-replication scheme upon table creation Output actionable warning message upon creation of a new table when the number of alive tablet servers is not enough to re-replicate tablet replicas in case of a failure. Since the 3-4-3 replication scheme is now enabled by default, this might be useful in case if running a cluster with just N tablet servers when newly created tables have replication factor of N (e.g., imagine running with just 3 tablet servers total and creating tables with the default replication factor of 3). This is a follow-up for c40e0587bf6a6aa55e5bd72dd2dd9356b1507f2e. Change-Id: I5a59451ad876d9864a90607187a05f8526cd8cbf --- M src/kudu/master/catalog_manager.cc 1 file changed, 36 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/9321/2 -- To view, visit http://gerrit.cloudera.org:8080/9321 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5a59451ad876d9864a90607187a05f8526cd8cbf Gerrit-Change-Number: 9321 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-2305: Fix local variable usage to handle 2GB messages
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9355 ) Change subject: KUDU-2305: Fix local variable usage to handle 2GB messages .. Patch Set 3: Code-Review+1 (1 comment) Would be good for Dan B to take a look at this as our resident int overflow detective http://gerrit.cloudera.org:8080/#/c/9355/3/src/kudu/rpc/serialization.cc File src/kudu/rpc/serialization.cc: http://gerrit.cloudera.org:8080/#/c/9355/3/src/kudu/rpc/serialization.cc@61 PS3, Line 61: static_cast(additional_size) > Yes, I think only the first cast is necessary to fix the ASAN issue. This s could go either way I guess. I often get confused by type promotion rules like this so I'm not against being explicit. -- To view, visit http://gerrit.cloudera.org:8080/9355 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8e468099b16f7eb55de71b753acae8f57d18287c Gerrit-Change-Number: 9355 Gerrit-PatchSet: 3 Gerrit-Owner: Joe McDonnellGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 23:10:25 + Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] check re-replication scheme upon table creation
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9321 ) Change subject: [catalog_manager] check re-replication scheme upon table creation .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/9321/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/9321/1/src/kudu/master/catalog_manager.cc@1386 PS1, Line 1386: alive nit: s/alive/live/ http://gerrit.cloudera.org:8080/#/c/9321/1/src/kudu/master/catalog_manager.cc@1401 PS1, Line 1401: . add: (not recommended) -- To view, visit http://gerrit.cloudera.org:8080/9321 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5a59451ad876d9864a90607187a05f8526cd8cbf Gerrit-Change-Number: 9321 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 20 Feb 2018 23:08:21 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9363 ) Change subject: KUDU-721: Support range partitions on decimal columns .. Patch Set 1: LGTM with Alexey's points addressed -- To view, visit http://gerrit.cloudera.org:8080/9363 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917 Gerrit-Change-Number: 9363 Gerrit-PatchSet: 1 Gerrit-Owner: Grant HenkeGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 20 Feb 2018 23:07:43 + Gerrit-HasComments: No
[kudu-CR] KUDU-721: Support range partitions on decimal columns
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9363 ) Change subject: KUDU-721: Support range partitions on decimal columns .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc@1639 PS1, Line 1639: u_schema_ nit: I don't think we use identifiers like 'x_' for local variables; would 'u_schema' suffice here? http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc@1643 PS1, Line 1643: CHECK_OK why not just ASSERT_OK() ? http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/client/client-test.cc@1645 PS1, Line 1645: unique_ptr lower_bound(u_schema_.NewRow()); : ASSERT_OK(lower_bound->SetUnscaledDecimal("key", -1)); : unique_ptr upper_bound(u_schema_.NewRow()); : ASSERT_OK(upper_bound->SetUnscaledDecimal("key", 99)); I think it would be nice to add a 'negative' test case (maybe, in some different scenario) to test for: * swapped upper/lower bounds (e.g., upper -1, lower 99) * bounds that lead to an empty partition (e.g. lower 10, upper 10) * bounds with MAX and MIN values http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/9363/1/src/kudu/common/partition.cc@1043 PS1, Line 1043: ColumnSchema const reference? -- To view, visit http://gerrit.cloudera.org:8080/9363 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I419d442dd233c73166c6e391907c8443ebecc917 Gerrit-Change-Number: 9363 Gerrit-PatchSet: 1 Gerrit-Owner: Grant HenkeGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 20 Feb 2018 22:59:18 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/7/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/7/src/kudu/rpc/rpc-test.cc@494 PS7, Line 494: ASSERT_GT(dump_resp.outbound_connections(0).outbound_queue_size(), 0); Can you try looping this test with --stress-cpu-threads=$(nproc) to double check it isn't flaky? -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 7 Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 22:38:19 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc@489 PS5, Line 489: > I still don't think that helps, because the RPCs will still end up read off Ah right, that was a mistake. I changed it to keep the server's reactor thread busy by adding a sleep task for 2 seconds before queuing up RPCs. I tried using a latch, but the reactor thread restrictions hits a DCHECK, as we're not supposed to block on reactor threads. -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 7 Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 22:36:30 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#7). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 66 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/7 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 7 Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [TestWorkload] an option to retry on read timeouts
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9295 ) Change subject: [TestWorkload] an option to retry on read timeouts .. Patch Set 4: Thanks you Todd and Mike for the reviews. Yes, I agree it makes sense to verify if that was an exposure of some issues on the client side. I'll take a look at that a bit later, maybe in the scope of wrapping up along with https://gerrit.cloudera.org/c/7037/ -- To view, visit http://gerrit.cloudera.org:8080/9295 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I54a6b81e4ba3a55676f1fc97cd577a5fe545c550 Gerrit-Change-Number: 9295 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 22:29:09 + Gerrit-HasComments: No
[kudu-CR] KUDU-2297 (part 1): refactor metrics log out of ServerBase
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9326 ) Change subject: KUDU-2297 (part 1): refactor metrics log out of ServerBase .. Patch Set 3: (2 comments) SGTM except nits. Release failure looks unrelated. The TSAN failure may be legit, though. http://gerrit.cloudera.org:8080/#/c/9326/3/src/kudu/server/diagnostics_log.cc File src/kudu/server/diagnostics_log.cc: http://gerrit.cloudera.org:8080/#/c/9326/3/src/kudu/server/diagnostics_log.cc@104 PS3, Line 104: Unable to collect metrics to diagnostics log Add a bit saying we'll retry in `kWaitBetweenFailures` seconds. http://gerrit.cloudera.org:8080/#/c/9326/3/src/kudu/server/diagnostics_log.cc@118 PS3, Line 118: this seems redundant with the "only include changed metrics" above Referring to `opts.only_modified_in_or_after_epoch` below? -- To view, visit http://gerrit.cloudera.org:8080/9326 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f7bc7335b1f4c2a5c62fc5fe672289d5027a10d Gerrit-Change-Number: 9326 Gerrit-PatchSet: 3 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 20 Feb 2018 22:27:06 + Gerrit-HasComments: Yes
[kudu-CR] [TestWorkload] an option to retry on read timeouts
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9295 ) Change subject: [TestWorkload] an option to retry on read timeouts .. Patch Set 4: Code-Review+2 agreed w/ Todd use of this may be indicative of scanner retry bugs -- To view, visit http://gerrit.cloudera.org:8080/9295 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I54a6b81e4ba3a55676f1fc97cd577a5fe545c550 Gerrit-Change-Number: 9295 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 22:24:32 + Gerrit-HasComments: No
[kudu-CR] Simplify OpId/Timestamp assignment and make it atomic
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7221 ) Change subject: Simplify OpId/Timestamp assignment and make it atomic .. Patch Set 17: (10 comments) http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/quorum_util.cc@158 PS17, Line 158: std::set uuids; add: DCHECK(type == COMMITTED_CONFIG || type == PENDING_CONFIG) http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.h@449 PS17, Line 449: mus must http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@648 PS17, Line 648: RETURN_NOT_OK(HandlePendingConfigChangeUnlocked(round)); > although this is right, it looks confusing because round probably isn't a c or just: if (round->replicate_msg()->op_type() == CHANGE_CONFIG_OP) { ... } http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@662 PS17, Line 662: if (PREDICT_TRUE(round->replicate_msg()->op_type() != CHANGE_CONFIG_OP)) return Status::OK(); Due to the method name, seems less surprising to add: DCHECK_EQ(CHANGE_CONFIG_OP, round->replicate_msg()->op_type()); http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@673 PS17, Line 673: nit: extra line http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/consensus/raft_consensus.cc@691 PS17, Line 691: ( nit: no need for inner parentheses http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/tablet/transactions/transaction_driver.cc File src/kudu/tablet/transactions/transaction_driver.cc: http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/tablet/transactions/transaction_driver.cc@336 PS17, Line 336: rule s/rule/invariant/ http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/tablet/transactions/transaction_driver.cc@360 PS17, Line 360: constraint s/constraint/above invariant/ By the way: is there a DCHECK somewhere in mvcc to enforce the invariant that clean time < any tx timestamp? http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/tablet/transactions/transaction_driver.cc@371 PS17, Line 371: repl_state_copy This variable masks a variable of the same name in an outer scope http://gerrit.cloudera.org:8080/#/c/7221/17/src/kudu/tablet/transactions/transaction_driver.cc@384 PS17, Line 384: case REPLICATING can we still get to this case? -- To view, visit http://gerrit.cloudera.org:8080/7221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2bedcc5c3708d06f48131f34ea14c835867b4800 Gerrit-Change-Number: 7221 Gerrit-PatchSet: 17 Gerrit-Owner: David Ribeiro AlvesGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 22:20:34 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc@489 PS5, Line 489: controllers.back().get(), boost::bind(::CountDown, boost::ref(latch))); > Yes you're right. I can't think of a sure shot way to make sure that we don I still don't think that helps, because the RPCs will still end up read off the wire by the reactor thread and thus won't be pending in the client outbound queue, even if the server is blocked _processing them_. You'd have to somehow block the server's reactor thread so that it isn't reading off the pipe, and then you'd have to fill up the socket's send-buffer so that the client can't make progress sending the transfers to the pipe -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 21:55:09 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 6: > Build Failed > > http://jenkins.kudu.apache.org/job/kudu-gerrit/12071/ : FAILURE Failure is in an unrelated flaky test. -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 21:45:42 + Gerrit-HasComments: No
[kudu-CR] KUDU-2275. Upgrade libunwind to 1.3-rc1
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9335 ) Change subject: KUDU-2275. Upgrade libunwind to 1.3-rc1 .. KUDU-2275. Upgrade libunwind to 1.3-rc1 Per KUDU-2275, the version of libunwind that we were previously using can occasionally crash during stack unwinding if it attempts to access a page which is mprotected to be non-readable. This could be due to incorrect interpretation of dwarf unwinding info or some other bug. As we are trying to collect stacks more aggressively now, it's important to upgrade. This new version uses a more robust mechanism for checking whether a page is valid to access before accessing it during unwinding. This also includes a patch from the libunwind git repo which fixes a potential issue with libunwind in ASAN builds. I didn't run into this issue yet myself, but seems like a straightforward and necessary fix. Change-Id: I3f942a0d566e1b1e5ebe48f09362bf8aa3e1e33e Reviewed-on: http://gerrit.cloudera.org:8080/9335 Tested-by: Kudu Jenkins Reviewed-by: Mike Percy--- M src/kudu/util/file_cache-test.cc M thirdparty/download-thirdparty.sh A thirdparty/patches/libunwind-Use-syscall-directly-in-write_validate-to-avoid-ASAN.patch M thirdparty/vars.sh 4 files changed, 61 insertions(+), 4 deletions(-) Approvals: Kudu Jenkins: Verified Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/9335 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I3f942a0d566e1b1e5ebe48f09362bf8aa3e1e33e Gerrit-Change-Number: 9335 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc@489 PS5, Line 489: controllers.back().get(), boost::bind(::CountDown, boost::ref(latch))); > Maybe I'm missing something here. If the main thread runs a bit slow before Yes you're right. I can't think of a sure shot way to make sure that we don't have this race. However, I've changed the RPC method to GenericCalculatorService::kSleepMethodName, with a 1 second sleep and 10 total RPCs. With 3 service threads (the default), that should give the main thread a little more than 3 seconds to dump the running RPCs information, which makes the flakiness very unlikely. I'm open to suggestions if this doesn't suffice. -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 21:21:46 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#6). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 66 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/6 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1704: add c++ client support for READ YOUR WRITES mode
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8823 ) Change subject: KUDU-1704: add c++ client support for READ_YOUR_WRITES mode .. Patch Set 9: (2 comments) http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client.h@1740 PS9, Line 1740: /// When @c READ_YOUR_WRITES is specified, the server will pick a timestamp to use I still think there is room to simplify these docs. I would prefer if they didn't mention timestamps _at all_. http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client.h@1745 PS9, Line 1745: If no propagated timestamp is provided the server What does this mean in terms of an application developer reading this doc? How would such a developer provide a timestamp? Why would such a developer want to provide a propagated timestamp? -- To view, visit http://gerrit.cloudera.org:8080/8823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34214245a78aed172a28fbdb395ff5bccd0fc0e1 Gerrit-Change-Number: 8823 Gerrit-PatchSet: 9 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 20 Feb 2018 20:51:58 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add c++ client support for READ YOUR WRITES mode
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8823 ) Change subject: KUDU-1704: add c++ client support for READ_YOUR_WRITES mode .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client-internal.h@278 PS8, Line 278: AtomicInt max_observed_timestamp_; > > As implemented here, isn't there interactions between two concurrently op IIUC the RYW scan mode picks a propagated timestamp up-front, and then propagates that same timestamp with every NewScanRequest RPC. To me, that implies that the timestamp should be kept in a field of the scanner, since it's a value specific to that scanner. Instead it looks like it's being kept as a field on the client, and therefore is shared among potentially many scanners. Isn't that problematic? Edit: wrote the above, then read David's bullet-pointed comment. I think he and I are basically getting to the same point. -- To view, visit http://gerrit.cloudera.org:8080/8823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34214245a78aed172a28fbdb395ff5bccd0fc0e1 Gerrit-Change-Number: 8823 Gerrit-PatchSet: 8 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 20 Feb 2018 20:47:44 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 ) Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode .. Patch Set 16: (4 comments) lgtm, just a nit and one last question http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2117 PS16, Line 2117: . s/./,/ http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118 PS16, Line 2118: initiated s/initiated with/default-constructed to/ http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118 PS16, Line 2118: Since s/Since/since/ http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163 PS16, Line 2163: (propagation ts + 1) should not is that guaranteed by something? -- To view, visit http://gerrit.cloudera.org:8080/8804 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48 Gerrit-Change-Number: 8804 Gerrit-PatchSet: 16 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 20 Feb 2018 20:47:44 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2275. Upgrade libunwind to 1.3-rc1
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9335 ) Change subject: KUDU-2275. Upgrade libunwind to 1.3-rc1 .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9335 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f942a0d566e1b1e5ebe48f09362bf8aa3e1e33e Gerrit-Change-Number: 9335 Gerrit-PatchSet: 2 Gerrit-Owner: Todd LipconGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 20 Feb 2018 20:29:28 + Gerrit-HasComments: No
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc@489 PS5, Line 489: ASSERT_GT(dump_resp.outbound_connections(0).outbound_queue_size(), 0); Maybe I'm missing something here. If the main thread runs a bit slow before DumpRunningRpcs calls, what is preventing the queue from immediately emptying before this assertion? eg if you added a sleep on line 486 wouldn't this test now fail? Even with 1000 RPCs it seems like they could very easily all get sent in a matter of a couple milliseconds -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 5 Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 20:29:12 + Gerrit-HasComments: Yes
[kudu-CR] Support decimal columns in load generation tool
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9362 ) Change subject: Support decimal columns in load generation tool .. Support decimal columns in load generation tool Change-Id: I8f9441ab3483775e74f5fdedfff309bdcbc8ab17 Reviewed-on: http://gerrit.cloudera.org:8080/9362 Reviewed-by: Alexey SerbinTested-by: Kudu Jenkins --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_perf.cc 2 files changed, 23 insertions(+), 0 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/9362 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I8f9441ab3483775e74f5fdedfff309bdcbc8ab17 Gerrit-Change-Number: 9362 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1704: add c++ client support for READ YOUR WRITES mode
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8823 ) Change subject: KUDU-1704: add c++ client support for READ_YOUR_WRITES mode .. Patch Set 9: (6 comments) http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-internal.h@197 PS9, Line 197: from servers that only for all : // opened scans with READ_YOUR_WRITES mode. this sentence is non-sensical. also max/latest is bound to be confusing. remind me again why we need the two? I thought we'd do something like: - At the beginning of a scan, get the last propagated timestamp (say t0) and pass it to the scan config (we'll always use this one and won't read the last propagated timestamp from the client for the duration of the scan). - With each scan request, for each tablet, send t0. Each server is free to choose a timestamp that is bigger than t0 exactly as implemented in your server side patch. - Each scan response includes the chosen timestamp on the server, ts. - Each scan response updates the "last propagated timestamp" with ts, making sure we have RYR. http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-internal.h@201 PS9, Line 201: // Updates the max observed timestamp from servers that only for all : // opened scan with READ_YOUR_WRITES mode, if the given timestamp is : // greater than the one stored. : void UpdateMaxObservedTimestamp(uint64_t timestamp); also non-sensical; also see my comment above http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client-test.cc@992 PS9, Line 992: // Check counts changed accordingly using both READ_LATEST and : // READ_YOUR_WRITES scan mode. : for (auto read_mode : kReadModes) { : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, kNoBound, kNoBound)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, kNoBound, 15)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 27, kNoBound)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 0, 15)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 0, 10)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 0, 20)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 0, 30)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 14, 30)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, 30, 30)); : ASSERT_EQ(0, CountRowsFromClient(table.get(), read_mode, kTabletsNum * kRowsPerTablet, : kNoBound)); : } can you make this a parameterized test that received the read mode as input? http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/client.h@1743 PS9, Line 1743: minimize nit: verbage (... ensures ... minimize sounds weird) http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/scan_token-internal.cc@253 PS9, Line 253: LOG(WARNING) << "Ignoring snapshot timestamp since not in " : "READ_AT_SNAPSHOT mode."; should we validate this on the config? i.e. don't allow to pass a snapshot timestamp (or to have on set) when choosing RYW? we could return an error status there and then LOG(FATAL) here http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/8823/9/src/kudu/client/scanner-internal.cc@330 PS9, Line 330: if (configuration_.has_snapshot_timestamp()) { : LOG(WARNING) << "Ignoring snapshot timestamp since " : "not in READ_AT_SNAPSHOT mode."; : } same comment I did elsewhere -- To view, visit http://gerrit.cloudera.org:8080/8823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34214245a78aed172a28fbdb395ff5bccd0fc0e1 Gerrit-Change-Number: 8823 Gerrit-PatchSet: 9 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alex Rodoni Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] internal mini cluster: support Cluster/LogVerifier
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9137 ) Change subject: internal_mini_cluster: support Cluster/LogVerifier .. Patch Set 12: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I228a6e3ba1a42db4e243ffdc5116f0c60ee04a84 Gerrit-Change-Number: 9137 Gerrit-PatchSet: 12 Gerrit-Owner: Andrew WongGerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 20:25:55 + Gerrit-HasComments: No
[kudu-CR] KUDU-2291 (part 2): Add a /stacks page
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9253 ) Change subject: KUDU-2291 (part 2): Add a /stacks page .. Patch Set 5: Verified+1 unrelated consensus test flke -- To view, visit http://gerrit.cloudera.org:8080/9253 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b Gerrit-Change-Number: 9253 Gerrit-PatchSet: 5 Gerrit-Owner: Todd LipconGerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 20 Feb 2018 20:22:25 + Gerrit-HasComments: No
[kudu-CR] KUDU-2291 (part 2): Add a /stacks page
Todd Lipcon has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/9253 ) Change subject: KUDU-2291 (part 2): Add a /stacks page .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/9253 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b Gerrit-Change-Number: 9253 Gerrit-PatchSet: 5 Gerrit-Owner: Todd LipconGerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Support decimal columns in load generation tool
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9362 ) Change subject: Support decimal columns in load generation tool .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9362 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f9441ab3483775e74f5fdedfff309bdcbc8ab17 Gerrit-Change-Number: 9362 Gerrit-PatchSet: 2 Gerrit-Owner: Grant HenkeGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 20 Feb 2018 18:17:38 + Gerrit-HasComments: No
[kudu-CR] Support decimal columns in load generation tool
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9362 ) Change subject: Support decimal columns in load generation tool .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/9362/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/9362/1/src/kudu/tools/kudu-tool-test.cc@1345 PS1, Line 1345: DECIMAL32 > DECIMAL64 ? Done http://gerrit.cloudera.org:8080/#/c/9362/1/src/kudu/tools/kudu-tool-test.cc@1348 PS1, Line 1348: DECIMAL32 > DECIMAL128 ? Done -- To view, visit http://gerrit.cloudera.org:8080/9362 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f9441ab3483775e74f5fdedfff309bdcbc8ab17 Gerrit-Change-Number: 9362 Gerrit-PatchSet: 1 Gerrit-Owner: Grant HenkeGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 20 Feb 2018 18:12:10 + Gerrit-HasComments: Yes
[kudu-CR] Support decimal columns in load generation tool
Hello Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9362 to look at the new patch set (#2). Change subject: Support decimal columns in load generation tool .. Support decimal columns in load generation tool Change-Id: I8f9441ab3483775e74f5fdedfff309bdcbc8ab17 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_perf.cc 2 files changed, 23 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/9362/2 -- To view, visit http://gerrit.cloudera.org:8080/9362 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8f9441ab3483775e74f5fdedfff309bdcbc8ab17 Gerrit-Change-Number: 9362 Gerrit-PatchSet: 2 Gerrit-Owner: Grant HenkeGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Support decimal columns in load generation tool
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9362 ) Change subject: Support decimal columns in load generation tool .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/9362/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/9362/1/src/kudu/tools/kudu-tool-test.cc@1345 PS1, Line 1345: DECIMAL32 DECIMAL64 ? http://gerrit.cloudera.org:8080/#/c/9362/1/src/kudu/tools/kudu-tool-test.cc@1348 PS1, Line 1348: DECIMAL32 DECIMAL128 ? -- To view, visit http://gerrit.cloudera.org:8080/9362 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f9441ab3483775e74f5fdedfff309bdcbc8ab17 Gerrit-Change-Number: 9362 Gerrit-PatchSet: 1 Gerrit-Owner: Grant HenkeGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 20 Feb 2018 18:05:12 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: [Flume] Add DECIMAL type support
Hello Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9365 to look at the new patch set (#2). Change subject: KUDU-721: [Flume] Add DECIMAL type support .. KUDU-721: [Flume] Add DECIMAL type support Adds decimal column support to the Flume KuduSink including the Regex and Avro operations producers. Note: Sets enableDecimalLogicalType to true in the Maven Avro plugin for code generation because the default is false. Change-Id: Ibc02d683dd1bce2cc0de255e4d072436b6c0163a --- M java/kudu-flume-sink/pom.xml M java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/AvroKuduOperationsProducer.java M java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java M java/kudu-flume-sink/src/test/avro/testAvroKuduOperationsProducer.avsc M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduOperationsProducerTest.java M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerTest.java 6 files changed, 64 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/65/9365/2 -- To view, visit http://gerrit.cloudera.org:8080/9365 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibc02d683dd1bce2cc0de255e4d072436b6c0163a Gerrit-Change-Number: 9365 Gerrit-PatchSet: 2 Gerrit-Owner: Grant HenkeGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#5). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 61 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/5 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 5 Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2305: Fix local variable usage to handle 2GB messages
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/9355 ) Change subject: KUDU-2305: Fix local variable usage to handle 2GB messages .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/9355/3/src/kudu/rpc/serialization.cc File src/kudu/rpc/serialization.cc: http://gerrit.cloudera.org:8080/#/c/9355/3/src/kudu/rpc/serialization.cc@61 PS3, Line 61: static_cast(additional_size) > Won't this be automatically promoted to int64_t ? Is this second cast neede Yes, I think only the first cast is necessary to fix the ASAN issue. This seems like a code style thing, so I thought I'd let the Kudu guys comment. I don't have a preference. http://gerrit.cloudera.org:8080/#/c/9355/3/src/kudu/rpc/transfer.cc File src/kudu/rpc/transfer.cc: http://gerrit.cloudera.org:8080/#/c/9355/3/src/kudu/rpc/transfer.cc@81 PS3, Line 81: kMsgLengthPrefixLength > Does it make sense to to DCHECK_LE(kMsgLengthPrefixLength, INT_MAX) or in t kMsgLengthPrefixLength is a uint8_t and is always 4, so rem is always a small number. The comment right above this is confusing. I'll see if I can make it clearer. -- To view, visit http://gerrit.cloudera.org:8080/9355 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8e468099b16f7eb55de71b753acae8f57d18287c Gerrit-Change-Number: 9355 Gerrit-PatchSet: 3 Gerrit-Owner: Joe McDonnellGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 17:24:39 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-721: [Flume] Add DECIMAL type support
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9365 Change subject: KUDU-721: [Flume] Add DECIMAL type support .. KUDU-721: [Flume] Add DECIMAL type support Adds decimal column support to the Flume KuduSink including the Regex and Avro operations producers. Change-Id: Ibc02d683dd1bce2cc0de255e4d072436b6c0163a --- M java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/AvroKuduOperationsProducer.java M java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducer.java M java/kudu-flume-sink/src/test/avro/testAvroKuduOperationsProducer.avsc M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/AvroKuduOperationsProducerTest.java M java/kudu-flume-sink/src/test/java/org/apache/kudu/flume/sink/RegexpKuduOperationsProducerTest.java 5 files changed, 61 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/65/9365/1 -- To view, visit http://gerrit.cloudera.org:8080/9365 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ibc02d683dd1bce2cc0de255e4d072436b6c0163a Gerrit-Change-Number: 9365 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke