[kudu-CR] KUDU-2599: fix flaky DefaultSourceTest.testSocketReadTimeoutPropagation
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11916 ) Change subject: KUDU-2599: fix flaky DefaultSourceTest.testSocketReadTimeoutPropagation .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11916/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala File java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala: http://gerrit.cloudera.org:8080/#/c/11916/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala@907 PS1, Line 907: 6 Edgy. -- To view, visit http://gerrit.cloudera.org:8080/11916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I81a20f776c115caf3bd720bc532bbeb5fc69bc42 Gerrit-Change-Number: 11916 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Fri, 09 Nov 2018 06:03:08 + Gerrit-HasComments: Yes
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11858 ) Change subject: (03/05) delta_store: support iteration with snap_to_exclude .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11858 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c Gerrit-Change-Number: 11858 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11858 ) Change subject: (03/05) delta_store: support iteration with snap_to_exclude .. Patch Set 3: Verified+1 Overriding Jenkins, the failure was known flake KUDU-2599. -- To view, visit http://gerrit.cloudera.org:8080/11858 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c Gerrit-Change-Number: 11858 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 09 Nov 2018 05:52:02 + Gerrit-HasComments: No
[kudu-CR] KUDU-2599: fix flaky DefaultSourceTest.testSocketReadTimeoutPropagation
Hello Mike Percy, Grant Henke, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11916 to review the following change. Change subject: KUDU-2599: fix flaky DefaultSourceTest.testSocketReadTimeoutPropagation .. KUDU-2599: fix flaky DefaultSourceTest.testSocketReadTimeoutPropagation This test is just about checking the propagation of an option from the DefaultSource to the KuduRelation. However, building a DataFrame involves a connection to the cluster, and if we set socketReadTimeoutMs to 1, it's going to be very difficult to connect, especially if we're testing TSAN-instrumented binaries whose thread creation is inherently slower. The fix? Use a magic number larger than '1'. It's that simple. Change-Id: I81a20f776c115caf3bd720bc532bbeb5fc69bc42 --- M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala 1 file changed, 8 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/11916/1 -- To view, visit http://gerrit.cloudera.org:8080/11916 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I81a20f776c115caf3bd720bc532bbeb5fc69bc42 Gerrit-Change-Number: 11916 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy
[kudu-CR] (03/05) delta store: support iteration with snap to exclude
Hello Mike Percy, David Ribeiro Alves, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11858 to look at the new patch set (#3). Change subject: (03/05) delta_store: support iteration with snap_to_exclude .. (03/05) delta_store: support iteration with snap_to_exclude This patch changes the delta stores (DMS and delta files) to respect snap_to_exclude during iteration. The key changes are: - The introduction of the "selection" criteria, a new delta relevancy formula for determining whether a delta applies to a scan with both snap_to_exclude and snap_to_include. The existing "application" criteria was formalized and moved into delta_relevancy.h. There was also a non-trivial change to DeltaFileReader::IsRelevantForSnapshot() to use both criterias when culling entire delta files. - A new SelectUpdates() method for using the selection criteria on a batch of prepared deltas. SelectUpdates() requires new in-memory state, the creation of which is gated behind a new PREPARE_FOR_SELECT flag so as not to affect regular scans. - Updates to the delta fuzz testing logic to test iterators with two timestamps, and to provide SelectUpdates() coverage. A future patch will modify DeltaApplier to use SelectUpdates for diff scans. Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c --- M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.h A src/kudu/tablet/delta_relevancy.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/delta_store.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/tablet-test-util.cc M src/kudu/tablet/tablet-test-util.h 11 files changed, 468 insertions(+), 92 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/11858/3 -- To view, visit http://gerrit.cloudera.org:8080/11858 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c Gerrit-Change-Number: 11858 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] (02/05) delta store: convert DeltaIterator::PrepareBatch flags into bitfield
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11857 ) Change subject: (02/05) delta_store: convert DeltaIterator::PrepareBatch flags into bitfield .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/11857/2/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/11857/2/src/kudu/tablet/delta_store.h@205 PS2, Line 205: // 'flags' is a bitfield describing what operation(s) the batch will be used > Nit: Is there a more descriptive name that could be used? prepairForFlags? Sure, I can rename it to something more specific. http://gerrit.cloudera.org:8080/#/c/11857/2/src/kudu/tablet/delta_store.h@349 PS2, Line 349: prepared_for > prepared_flags_ Done http://gerrit.cloudera.org:8080/#/c/11857/2/src/kudu/tablet/delta_store.h@369 PS2, Line 369: prepared_for_ > same Done -- To view, visit http://gerrit.cloudera.org:8080/11857 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a Gerrit-Change-Number: 11857 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 09 Nov 2018 04:58:40 + Gerrit-HasComments: Yes
[kudu-CR] (02/05) delta store: convert DeltaIterator::PrepareBatch flags into bitfield
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11857 to look at the new patch set (#3). Change subject: (02/05) delta_store: convert DeltaIterator::PrepareBatch flags into bitfield .. (02/05) delta_store: convert DeltaIterator::PrepareBatch flags into bitfield A follow-on patch will introduce DeltaIterator::SelectUpdates(), a new method for extracting delta information from a DeltaIterator. As it is only to be used by diff scans and will require dedicated in-memory state, it'll also come with a new DeltaIterator::PrepareBatch() flag. However, diff scans will need to use both SelectUpdates() and ApplyUpdates(), and it'd be a shame to have to reseek and reprepare just to use both extraction methods. As such, this patch makes it possible to prepare a batch for use in multiple delta extraction methods. Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a --- M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/delta_store.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/tablet-test-util.h 9 files changed, 55 insertions(+), 80 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/11857/3 -- To view, visit http://gerrit.cloudera.org:8080/11857 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a Gerrit-Change-Number: 11857 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] (01/05) delta store: avoid copying deleted row data in ApplyUpdates
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11856 ) Change subject: (01/05) delta_store: avoid copying deleted row data in ApplyUpdates .. (01/05) delta_store: avoid copying deleted row data in ApplyUpdates When applying deltas, the scan path will first populate a selection vector with deletes (i.e. a row is unset if there's a relevant DELETE for it), and then use it to enable two optimizations: 1. Short-circuit out if all rows in the block have been deleted. 2. Skip predicate evaluation and base data copying for deleted rows. To this we can add a third optimization: don't apply a delta whose mutation is to a deleted row. Note that it's not incorrect to apply such deltas (as we'll skip deleted rows when serializing the scan response to the client); it's just unnecessary work. I wrote a microbenchmark to evaluate the impact of this optimization. Below is the timing and perf stat output of the microbenchmark before and after applying the optimization (specifically, applying this patch, then commenting out the filtering in DeltaPrepare::ApplyUpdates for the first run, and uncommenting it for the next run). Time spent running 1000 scans with 0 deletes: real 0.523s user 0.524s sys 0.000s Time spent running 1000 scans with 10 deletes: real 0.515suser 0.516s sys 0.000s Time spent running 1000 scans with 100 deletes: real 0.512s user 0.512s sys 0.000s Time spent running 1000 scans with 1000 deletes: real 0.553s user 0.552s sys 0.000s Performance counter stats for 'bin/deltamemstore-test --gtest_filter=*Varying* --benchmark_num_passes=1000': 2201.029290 task-clock (msec) #0.991 CPUs utilized 5 context-switches #0.002 K/sec 0 cpu-migrations#0.000 K/sec 4,950 page-faults #0.002 M/sec 8,276,723,832 cycles#3.760 GHz 24,539,935,941 instructions #2.96 insn per cycle 4,709,709,705 branches # 2139.776 M/sec 12,631,579 branch-misses #0.27% of all branches 2.220370506 seconds time elapsed Time spent running 1000 scans with 0 deletes: real 0.474s user 0.475s sys 0.000s Time spent running 1000 scans with 10 deletes: real 0.475suser 0.472s sys 0.004s Time spent running 1000 scans with 100 deletes: real 0.478s user 0.476s sys 0.004s Time spent running 1000 scans with 1000 deletes: real 0.550s user 0.552s sys 0.000s Performance counter stats for 'bin/deltamemstore-test --gtest_filter=*Varying* --benchmark_num_passes=1000': 2074.795741 task-clock (msec) #0.990 CPUs utilized 23 context-switches #0.011 K/sec 1 cpu-migrations#0.000 K/sec 4,951 page-faults #0.002 M/sec 7,675,100,058 cycles#3.699 GHz 23,100,692,252 instructions #3.01 insn per cycle 4,539,777,117 branches # 2188.060 M/sec 11,819,267 branch-misses #0.26% of all branches 2.096193851 seconds time elapsed Note: I originally wrote this patch thinking it was necessary for diff scan correctness, but have since convinced myself that it's just an optimization. Change-Id: I6a646d0816a96e9aba486c0d0a1e6b4a7e15c144 Reviewed-on: http://gerrit.cloudera.org:8080/11856 Tested-by: Kudu Jenkins Reviewed-by: Grant Henke Reviewed-by: Mike Percy --- M src/kudu/tablet/delta_applier.cc M src/kudu/tablet/delta_applier.h M src/kudu/tablet/delta_iterator_merger.cc M src/kudu/tablet/delta_iterator_merger.h M src/kudu/tablet/delta_store.cc M src/kudu/tablet/delta_store.h M src/kudu/tablet/deltafile-test.cc M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/deltamemstore-test.cc M src/kudu/tablet/deltamemstore.cc M src/kudu/tablet/deltamemstore.h M src/kudu/tablet/tablet-test-util.h 13 files changed, 116 insertions(+), 46 deletions(-) Approvals: Kudu Jenkins: Verified Grant Henke: Looks good to me, but someone else must approve Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11856 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I6a646d0816a96e9aba486c0d0a1e6b4a7e15c144 Gerrit-Change-Number: 11856 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2378: fix another aligned load crash
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11911 ) Change subject: KUDU-2378: fix another aligned load crash .. KUDU-2378: fix another aligned load crash The TestKuduBackup.testRandomBackupAndRestore test from the kudu-backup project would crash a TSAN master from time to time, with a stack trace ending in Variant::Reset. After some debugging, this turned out to be a holdover from KUDU-2378, where clang emitted a movaps (Aligned Load) instruction on a value that wasn't aligned. The holdover was missed because it was a static_cast (the original search covered reinterpret_casts); I did a search for other bad casts but found none. Why TSAN? Probably because it's the only build to use libc++, whose std::string uses the Small String Optimization to embed the data into the string itself, thus raising the possibility of the string's data being unaligned. By comparison, libstdc++'s COW std::string uses a separate allocation for string data, which is likely always 16-byte aligned. The patch includes a unit test that would repro the crash 100% of the time when built for TSAN. Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Reviewed-on: http://gerrit.cloudera.org:8080/11911 Tested-by: Adar Dembo Reviewed-by: Todd Lipcon --- M src/kudu/common/types.h M src/kudu/common/wire_protocol-test.cc 2 files changed, 11 insertions(+), 1 deletion(-) Approvals: Adar Dembo: Verified Todd Lipcon: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Gerrit-Change-Number: 11911 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2378: fix another aligned load crash
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11911 ) Change subject: KUDU-2378: fix another aligned load crash .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Gerrit-Change-Number: 11911 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 09 Nov 2018 02:07:31 + Gerrit-HasComments: No
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 ) Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs .. Patch Set 4: (2 comments) Looked a bit into the code, so far just a few nits. I don't understand the whole picture yet, but I'll try to digest the design doc and look once more over the weekend. http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy.cc File src/kudu/tablet/compaction_policy.cc: http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy.cc@126 PS4, Line 126: const RowSetInfo* nit: not related to this particular patch, but if item_type is typedef'ed, why not to use it in the signature of get_weight() and get_value() functions? http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.cc File src/kudu/tablet/rowset_info.cc: http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.cc@65 PS4, Line 65: compaction_small_rowset_tradeoff nit: is it worth marking this flag with the 'runtime' tag? -- To view, visit http://gerrit.cloudera.org:8080/11869 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c Gerrit-Change-Number: 11869 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 09 Nov 2018 01:39:17 + Gerrit-HasComments: Yes
[kudu-CR] Add "network plane" as part of ConnectionId
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 ) Change subject: Add "network_plane" as part of ConnectionId .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@696 PS4, Line 696: > Just copied and pasted that pattern from TestCredentialsPolicy. I don't see Was cleaning my inbox and found this thread. Just FYI: the decision to use mixed ASSERT and EXPECT in TestCredentialsPolicy is to spot more details if would a failure happen. Without asserting on OK return status of a function that outputs some metrics it does not make sense to look at those, however, I think it's beneficial to see all mismatches, not just the first one. That's about collecting more information on the whole picture. -- To view, visit http://gerrit.cloudera.org:8080/11681 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2 Gerrit-Change-Number: 11681 Gerrit-PatchSet: 4 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 09 Nov 2018 01:05:37 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2378: fix another aligned load crash
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11911 ) Change subject: KUDU-2378: fix another aligned load crash .. Patch Set 4: Verified+1 Overriding Jenkins, DefaultSourceTest failed in TSAN again. -- To view, visit http://gerrit.cloudera.org:8080/11911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Gerrit-Change-Number: 11911 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 09 Nov 2018 01:03:26 + Gerrit-HasComments: No
[kudu-CR] KUDU-2378: fix another aligned load crash
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11911 ) Change subject: KUDU-2378: fix another aligned load crash .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Gerrit-Change-Number: 11911 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [java] Upgrade to Spark 2.4
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11912 ) Change subject: [java] Upgrade to Spark 2.4 .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11912 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id1f8de543276c4dc82a57c4a2228ae2374f2d87f Gerrit-Change-Number: 11912 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 09 Nov 2018 00:22:53 + Gerrit-HasComments: No
[kudu-CR] (02/05) delta store: convert DeltaIterator::PrepareBatch flags into bitfield
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11857 ) Change subject: (02/05) delta_store: convert DeltaIterator::PrepareBatch flags into bitfield .. Patch Set 2: (2 comments) nice little code reduction in the test code http://gerrit.cloudera.org:8080/#/c/11857/2/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/11857/2/src/kudu/tablet/delta_store.h@349 PS2, Line 349: prepared_for prepared_flags_ http://gerrit.cloudera.org:8080/#/c/11857/2/src/kudu/tablet/delta_store.h@369 PS2, Line 369: prepared_for_ same -- To view, visit http://gerrit.cloudera.org:8080/11857 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a Gerrit-Change-Number: 11857 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 23:53:12 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2378: fix another aligned load crash
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11911 ) Change subject: KUDU-2378: fix another aligned load crash .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/11911/2/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/11911/2/src/kudu/common/types.h@702 PS2, Line 702: numeric_.u64 = *static_cast(value); > mind fixing all these others, too, so that whenever some poor soul tries to Todd and I talked offline. He started on a patch to fix all aligned loads, but decided it wasn't a high priority fix and shelved it. He gave his blessing to this patch which just fixes the int128 case, which is actually critical for x86. -- To view, visit http://gerrit.cloudera.org:8080/11911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Gerrit-Change-Number: 11911 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 23:40:33 + Gerrit-HasComments: Yes
[kudu-CR] [java] Upgrade to Spark 2.4
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11912 ) Change subject: [java] Upgrade to Spark 2.4 .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11912 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id1f8de543276c4dc82a57c4a2228ae2374f2d87f Gerrit-Change-Number: 11912 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 23:42:13 + Gerrit-HasComments: No
[kudu-CR] KUDU-2378: fix another aligned load crash
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11911 to look at the new patch set (#4). Change subject: KUDU-2378: fix another aligned load crash .. KUDU-2378: fix another aligned load crash The TestKuduBackup.testRandomBackupAndRestore test from the kudu-backup project would crash a TSAN master from time to time, with a stack trace ending in Variant::Reset. After some debugging, this turned out to be a holdover from KUDU-2378, where clang emitted a movaps (Aligned Load) instruction on a value that wasn't aligned. The holdover was missed because it was a static_cast (the original search covered reinterpret_casts); I did a search for other bad casts but found none. Why TSAN? Probably because it's the only build to use libc++, whose std::string uses the Small String Optimization to embed the data into the string itself, thus raising the possibility of the string's data being unaligned. By comparison, libstdc++'s COW std::string uses a separate allocation for string data, which is likely always 16-byte aligned. The patch includes a unit test that would repro the crash 100% of the time when built for TSAN. Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 --- M src/kudu/common/types.h M src/kudu/common/wire_protocol-test.cc 2 files changed, 11 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/11911/4 -- To view, visit http://gerrit.cloudera.org:8080/11911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Gerrit-Change-Number: 11911 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2378: fix another aligned load crash
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11911 ) Change subject: KUDU-2378: fix another aligned load crash .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11911/2/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/11911/2/src/kudu/common/types.h@702 PS2, Line 702: numeric_.u64 = *static_cast(value); mind fixing all these others, too, so that whenever some poor soul tries to port kudu to Alpha or something, it won't crash? -- To view, visit http://gerrit.cloudera.org:8080/11911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Gerrit-Change-Number: 11911 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 22:18:35 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2378: fix another aligned load crash
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11911 ) Change subject: KUDU-2378: fix another aligned load crash .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Gerrit-Change-Number: 11911 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 22:10:08 + Gerrit-HasComments: No
[kudu-CR] [cmake] no empty KUDU DATA FILES ctest env variables
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11907 ) Change subject: [cmake] no empty KUDU_DATA_FILES ctest env variables .. [cmake] no empty KUDU_DATA_FILES ctest env variables This patch introduces an update on ADD_KUDU_TEST() custom function: if DATA_FILES is not specified for a test, don't add KUDU_DATA_FILES variable into its ctest environment. Prior to this patch, ctest environment of every test binary would contain KUDU_DATA_FILES variable. If DATA_FILES was specified for a test, it was set to corresponding value, otherwise it was set to an empty string. Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb Reviewed-on: http://gerrit.cloudera.org:8080/11907 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M CMakeLists.txt 1 file changed, 4 insertions(+), 2 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11907 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb Gerrit-Change-Number: 11907 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [cmake] no empty KUDU DATA FILES ctest env variables
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11907 ) Change subject: [cmake] no empty KUDU_DATA_FILES ctest env variables .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11907/2/CMakeLists.txt File CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/11907/2/CMakeLists.txt@876 PS2, Line 876: If the DATA_FILES_LIST is : # empty, do not add the KUDU_DATA_FILES environment variable. > FWIW, I think this is obvious from the code. Done -- To view, visit http://gerrit.cloudera.org:8080/11907 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb Gerrit-Change-Number: 11907 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 00:30:22 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2378: fix another aligned load crash
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11911 to look at the new patch set (#3). Change subject: KUDU-2378: fix another aligned load crash .. KUDU-2378: fix another aligned load crash The TestKuduBackup.testRandomBackupAndRestore test from the kudu-backup project would crash a TSAN master from time to time, with a stack trace ending in Variant::Reset. After some debugging, this turned out to be a holdover from KUDU-2378, where clang emitted a movaps (Aligned Load) instruction on a value that wasn't aligned. The holdover was missed because it was a static_cast (the original search covered reinterpret_casts); I did a search for other bad casts but found none. Why TSAN? Probably because it's the only build to use libc++, whose std::string uses the Small String Optimization to embed the data into the string itself, thus raising the possibility of the string's data being unaligned. By comparison, libstdc++'s COW std::string uses a separate allocation for string data, which is likely always 16-byte aligned. The patch includes a unit test that would repro the crash 100% of the time when built for TSAN. Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 --- M src/kudu/common/types.h M src/kudu/common/wire_protocol-test.cc 2 files changed, 10 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/11911/3 -- To view, visit http://gerrit.cloudera.org:8080/11911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Gerrit-Change-Number: 11911 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2378: fix another aligned load crash
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11911 ) Change subject: KUDU-2378: fix another aligned load crash .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/11911/2/src/kudu/common/wire_protocol-test.cc File src/kudu/common/wire_protocol-test.cc: http://gerrit.cloudera.org:8080/#/c/11911/2/src/kudu/common/wire_protocol-test.cc@455 PS2, Line 455: pb.set_read_default_value(string(16, 'a')); > warning: length is bigger then string literal size [bugprone-string-constru Done -- To view, visit http://gerrit.cloudera.org:8080/11911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Gerrit-Change-Number: 11911 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 22:23:50 + Gerrit-HasComments: Yes
[kudu-CR] [dist-test] build environment for tests in loop mode
Hello Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11905 to look at the new patch set (#2). Change subject: [dist-test] build environment for tests in loop mode .. [dist-test] build environment for tests in loop mode This patch fixes the issue of not deploying corresponding DATA_FILES by dest-test when running a test via the loop sub-command. I used the following command to run the ts_location_assignment-itest in dist-test: KUDU_ALLOW_SLOW_TESTS=1 \ ../../build-support/dist_test.py loop -n 32 \ ./bin/ts_location_assignment-itest --stress_cpu_threads=16 Prior to this patch, all runs failed because of the absence of assign-location.py script at dist-test slaves: http://dist-test.cloudera.org/job?job_id=aserbin.1541627808.8037 With this patch, all runs succeeded: http://dist-test.cloudera.org/job?job_id=aserbin.1541627254.115588 Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d --- M build-support/dist_test.py 1 file changed, 47 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/11905/2 -- To view, visit http://gerrit.cloudera.org:8080/11905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d Gerrit-Change-Number: 11905 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin
[kudu-CR] Add "network plane" as part of ConnectionId
Hello Kudu Jenkins, Adar Dembo, Sailesh Mukil, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11681 to look at the new patch set (#5). Change subject: Add "network_plane" as part of ConnectionId .. Add "network_plane" as part of ConnectionId The motivation for doing so is to allow N services on the same host to be multiplexed on M different connections. For instance, a server may host multiple KRPC based services: one for control command and one for data transfer. Separating the connections between the control channel and the data channel prevents unnecessary delays of the control commands due to being stuck behind large data transfers from client to server. By default, the network_plane of a new ConnectionId is not set. A user can change it to a different value by calling Proxy::set_network_plane() on the ConnectionId. Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2 --- M src/kudu/rpc/connection_id.cc M src/kudu/rpc/connection_id.h M src/kudu/rpc/messenger.h M src/kudu/rpc/proxy.cc M src/kudu/rpc/proxy.h M src/kudu/rpc/reactor.h M src/kudu/rpc/rpc-test.cc 7 files changed, 138 insertions(+), 31 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/11681/5 -- To view, visit http://gerrit.cloudera.org:8080/11681 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2 Gerrit-Change-Number: 11681 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [cmake] no empty KUDU DATA FILES ctest env variables
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11907 to look at the new patch set (#3). Change subject: [cmake] no empty KUDU_DATA_FILES ctest env variables .. [cmake] no empty KUDU_DATA_FILES ctest env variables This patch introduces an update on ADD_KUDU_TEST() custom function: if DATA_FILES is not specified for a test, don't add KUDU_DATA_FILES variable into its ctest environment. Prior to this patch, ctest environment of every test binary would contain KUDU_DATA_FILES variable. If DATA_FILES was specified for a test, it was set to corresponding value, otherwise it was set to an empty string. Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb --- M CMakeLists.txt 1 file changed, 4 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11907/3 -- To view, visit http://gerrit.cloudera.org:8080/11907 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb Gerrit-Change-Number: 11907 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [java] Upgrade to Spark 2.4
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11912 Change subject: [java] Upgrade to Spark 2.4 .. [java] Upgrade to Spark 2.4 In Spark 2.4 spark-avro is now a part of Spark itself. This change migrates the Kudu spark-avro dependencies and adds a test to ensure that the functionality does not break. Change-Id: Id1f8de543276c4dc82a57c4a2228ae2374f2d87f --- M java/gradle/dependencies.gradle M java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala 2 files changed, 96 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/11912/1 -- To view, visit http://gerrit.cloudera.org:8080/11912 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Id1f8de543276c4dc82a57c4a2228ae2374f2d87f Gerrit-Change-Number: 11912 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] KUDU-2378: fix another aligned load crash
Hello Alexey Serbin, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11911 to look at the new patch set (#2). Change subject: KUDU-2378: fix another aligned load crash .. KUDU-2378: fix another aligned load crash The TestKuduBackup.testRandomBackupAndRestore test from the kudu-backup project would crash a TSAN master from time to time, with a stack trace ending in Variant::Reset. After some debugging, this turned out to be a holdover from KUDU-2378, where clang emitted a movaps (Aligned Load) instruction on a value that wasn't aligned. The holdover was missed because it was a static_cast (the original search covered reinterpret_casts); I did a search for other bad casts but found none. Why TSAN? Probably because it's the only build to use libc++, whose std::string uses the Small String Optimization to embed the data into the string itself, thus raising the possibility of the string's data being unaligned. By comparison, libstdc++'s COW std::string uses a separate allocation for string data, which is likely always 16-byte aligned. The patch includes a unit test that would repro the crash 100% of the time when built for TSAN. Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 --- M src/kudu/common/types.h M src/kudu/common/wire_protocol-test.cc 2 files changed, 10 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/11911/2 -- To view, visit http://gerrit.cloudera.org:8080/11911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Gerrit-Change-Number: 11911 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [cmake] no empty KUDU DATA FILES ctest env variables
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11907 to look at the new patch set (#2). Change subject: [cmake] no empty KUDU_DATA_FILES ctest env variables .. [cmake] no empty KUDU_DATA_FILES ctest env variables This patch introduces an update on ADD_KUDU_TEST() custom function: if DATA_FILES is not specified for a test, don't add KUDU_DATA_FILES variable into its ctest environment. Prior to this patch, ctest environment of every test binary would contain KUDU_DATA_FILES variable. If DATA_FILES was specified for a test, it was set to corresponding value, otherwise it was set to an empty string. Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb --- M CMakeLists.txt 1 file changed, 8 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11907/2 -- To view, visit http://gerrit.cloudera.org:8080/11907 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb Gerrit-Change-Number: 11907 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2378: fix another aligned load crash
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11911 ) Change subject: KUDU-2378: fix another aligned load crash .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Gerrit-Change-Number: 11911 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 21:27:00 + Gerrit-HasComments: No
[kudu-CR] run-test: don't report leaks that don't fail test
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 ) Change subject: run-test: don't report leaks that don't fail test .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11886/4/src/kudu/server/generic_service.cc File src/kudu/server/generic_service.cc: http://gerrit.cloudera.org:8080/#/c/11886/4/src/kudu/server/generic_service.cc@183 PS4, Line 183: LOG(INFO) << "LeakSanitizer found false positives. Ignoring them."; May want to make this bigger so it stands out from the report. And maybe WARNING instead of INFO. -- To view, visit http://gerrit.cloudera.org:8080/11886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 Gerrit-Change-Number: 11886 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 07 Nov 2018 23:50:37 + Gerrit-HasComments: Yes
[kudu-CR] [cmake] no empty KUDU DATA FILES ctest env variables
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11907 to look at the new patch set (#4). Change subject: [cmake] no empty KUDU_DATA_FILES ctest env variables .. [cmake] no empty KUDU_DATA_FILES ctest env variables This patch introduces an update on ADD_KUDU_TEST() custom function: if DATA_FILES is not specified for a test, don't add KUDU_DATA_FILES variable into its ctest environment. Prior to this patch, ctest environment of every test binary would contain KUDU_DATA_FILES variable. If DATA_FILES was specified for a test, it was set to corresponding value, otherwise it was set to an empty string. Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb --- M CMakeLists.txt 1 file changed, 4 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11907/4 -- To view, visit http://gerrit.cloudera.org:8080/11907 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb Gerrit-Change-Number: 11907 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] gradle: expose testRandomSeed property
Hello Grant Henke, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11908 to review the following change. Change subject: gradle: expose testRandomSeed property .. gradle: expose testRandomSeed property In order to read a system property in tests, it isn't enough to add a System.getProperty() call to the test; we need to expose the property in gradle so that it's properly passed through to the test. With this patch, testRandomSeed can be set using either -D or -P. Note that even with testRandomSeed overridden, the PRNG didn't produce deterministic results in TestKuduBackup.testRandomBackupAndRestore, but perhaps that's a quirk of Scala's Random class. Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d --- M java/gradle/tests.gradle 1 file changed, 5 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/11908/1 -- To view, visit http://gerrit.cloudera.org:8080/11908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d Gerrit-Change-Number: 11908 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke
[kudu-CR] run-test: don't report leaks that don't fail test
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11886 to look at the new patch set (#5). Change subject: run-test: don't report leaks that don't fail test .. run-test: don't report leaks that don't fail test LeakSanitizer will report a leak when allocating a string in SuperviseThread. It's unclear why this is the case, but upon inspecting the code, it seems like a false positive. The stack trace is as follows: = ==93677==ERROR: LeakSanitizer: detected memory leaks Direct leak of 58 byte(s) in 1 object(s) allocated from: #0 0x5318c8 in operator new(unsigned long) /data/8/awong/kudu/thirdparty/src/llvm-6.0.0.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:92 #1 0x3ae3a9c3c8 in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator const&) (/usr/lib64/libstdc++.so.6+0x3ae3a9c3c8) #2 0x3ae3a9d19a in std::string::_Rep::_M_clone(std::allocator const&, unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d19a) #3 0x3ae3a9d5eb in std::string::reserve(unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d5eb) #4 0x3ae3a9d770 in std::string::append(unsigned long, char) (/usr/lib64/libstdc++.so.6+0x3ae3a9d770) #5 0x7f518f799c12 in strings::SubstituteAndAppend(std::string*, StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.cc:110:3 #6 0x536e76 in strings::Substitute(StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.h:188:3 #7 0x7f5190590860 in kudu::Thread::SuperviseThread(void*) ../src/kudu/util/thread.cc:607:17 #8 0x3ae0e079d0 in start_thread (/lib64/libpthread.so.0+0x3ae0e079d0) #9 0x3ae0ae88fc in clone (/lib64/libc.so.6+0x3ae0ae88fc) This appears to be affecting a number tests, but generally only lines #0 and #1 are present in the logs, making them difficult to debug (a developer would have to rerun the test with specific ASAN_OPTIONS to unwind the stacktrace more). Namely, exactly_once_writes-itest (KUDU-2517), kudu-admin-test (KUDU-2583), and rebalancer-tool-test (untracked via Jira) all show the top of the above stack trace, and based on the full stack trace, it seems these are all false positives. The presence of issues like https://github.com/google/sanitizers/issues/757 confirms that LeakSanitizer can report false positives in workloads with high concurrency. Generally, the test binary will return an error in the face of real leaks, but in tests like the ones mentioned, the test may log messages reporting leaks, but not actually return an error because the "leak" was transient (e.g. see GenericServiceImpl::CheckLeaks). We currently inject errors into JUnit XML report if any leaks are reported, even for false positives, since the leak messages still find their way into the logs. This patch updates this to only inject these errors if the test also returned an error. For clarity, I also threw in a log statement to GenericServiceImpl::CheckLeaks denoting false positives. Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 --- M build-support/run-test.sh M src/kudu/server/generic_service.cc 2 files changed, 15 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/11886/5 -- To view, visit http://gerrit.cloudera.org:8080/11886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 Gerrit-Change-Number: 11886 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [cmake] no empty KUDU DATA FILES ctest env variables
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11907 ) Change subject: [cmake] no empty KUDU_DATA_FILES ctest env variables .. Patch Set 4: > (1 comment) Yes, if(DATA_FILES_LIST) works, even if DATA_FILES is set to "" in ADD_KUDU_TEST macro. -- To view, visit http://gerrit.cloudera.org:8080/11907 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb Gerrit-Change-Number: 11907 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 02:34:01 + Gerrit-HasComments: No
[kudu-CR] run-test: don't report leaks that don't fail test
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 ) Change subject: run-test: don't report leaks that don't fail test .. Patch Set 7: Turns out IWYU was unhappy too. -- To view, visit http://gerrit.cloudera.org:8080/11886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 Gerrit-Change-Number: 11886 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 02:34:14 + Gerrit-HasComments: No
[kudu-CR] revert change to exactly once writes-itest
Andrew Wong has abandoned this change. ( http://gerrit.cloudera.org:8080/11836 ) Change subject: revert change to exactly_once_writes-itest .. Abandoned This "leak" was a false positive, see c5665046cb9f441e096650b8d10154598cf80fb8 for more details -- To view, visit http://gerrit.cloudera.org:8080/11836 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: If4091d3905d871acb48ec4d88c7b81ee48bf0eed Gerrit-Change-Number: 11836 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] [dist-test] build environment for tests in loop mode
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11905 ) Change subject: [dist-test] build environment for tests in loop mode .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d Gerrit-Change-Number: 11905 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 20:10:01 + Gerrit-HasComments: No
[kudu-CR] [dist-test] build environment for tests in loop mode
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11905 )
Change subject: [dist-test] build environment for tests in loop mode
..
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11905/3/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/11905/3/build-support/dist_test.py@534
PS3, Line 534: cular subset of tests provided by the binary. In that case it
> I'll punt on this because from paranoid's point of view that looks fragile:
Ack
http://gerrit.cloudera.org:8080/#/c/11905/3/build-support/dist_test.py@537
PS3, Line 537: yield noise. To avoid this, we disable sharding in this case.
: filter_flags = [e for e in options.args
: if e.find("--gtest_filter")
> It's necessary to get just one out of many if it's necessary to avoid shard
Thanks for the comment!
--
To view, visit http://gerrit.cloudera.org:8080/11905
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d
Gerrit-Change-Number: 11905
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 20:12:48 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2378: fix another aligned load crash
Hello Alexey Serbin, Grant Henke, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11911 to review the following change. Change subject: KUDU-2378: fix another aligned load crash .. KUDU-2378: fix another aligned load crash The TestKuduBackup.testRandomBackupAndRestore test from the kudu-backup project would crash a TSAN master from time to time, with a stack trace ending in Variant::Reset. After some debugging, this turned out to be a holdover from KUDU-2378, where clang emitted a movaps (Aligned Load) instruction on a value that wasn't aligned. The holdover was missed because it was a static_cast (the original search covered reinterpret_casts); I did a search for other bad casts but found none. Why TSAN? Probably because it's the only build to use libc++, whose std::string uses the Small String Optimization to embed the data into the string itself, thus raising the possibility of the string's data being unaligned. By comparison, libstdc++'s COW std::string uses a separate allocation for string data, which is likely always 16-byte aligned. The patch includes a unit test that isn't totally necessary, but was a useful way to repro the crash in relative isolation. Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 --- M src/kudu/common/types.h M src/kudu/master/master-test.cc 2 files changed, 21 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/11911/1 -- To view, visit http://gerrit.cloudera.org:8080/11911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357 Gerrit-Change-Number: 11911 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [dist-test] build environment for tests in loop mode
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11905 ) Change subject: [dist-test] build environment for tests in loop mode .. [dist-test] build environment for tests in loop mode This patch fixes the issue of not deploying corresponding DATA_FILES by dist-test when running a test via the loop sub-command. I used the following command to run the ts_location_assignment-itest in dist-test: KUDU_ALLOW_SLOW_TESTS=1 \ ../../build-support/dist_test.py loop -n 32 \ ./bin/ts_location_assignment-itest --stress_cpu_threads=16 Prior to this patch, all runs failed because of the absence of assign-location.py script at dist-test slaves: http://dist-test.cloudera.org/job?job_id=aserbin.1541627808.8037 With this patch, all runs succeeded: http://dist-test.cloudera.org/job?job_id=aserbin.1541627254.115588 Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d Reviewed-on: http://gerrit.cloudera.org:8080/11905 Reviewed-by: Adar Dembo Tested-by: Alexey Serbin --- M build-support/dist_test.py 1 file changed, 54 insertions(+), 20 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Alexey Serbin: Verified -- To view, visit http://gerrit.cloudera.org:8080/11905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d Gerrit-Change-Number: 11905 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong
[kudu-CR] [dist-test] build environment for tests in loop mode
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11905 ) Change subject: [dist-test] build environment for tests in loop mode .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d Gerrit-Change-Number: 11905 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong
[kudu-CR] [dist-test] build environment for tests in loop mode
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11905 ) Change subject: [dist-test] build environment for tests in loop mode .. Patch Set 4: Verified+1 Unrelated flake in org.apache.kudu.spark.kudu.DefaultSourceTest -- To view, visit http://gerrit.cloudera.org:8080/11905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d Gerrit-Change-Number: 11905 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Comment-Date: Thu, 08 Nov 2018 20:41:17 + Gerrit-HasComments: No
[kudu-CR] Add "network plane" as part of ConnectionId
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 ) Change subject: Add "network_plane" as part of ConnectionId .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7 PS1, Line 7: Add "service_name" as part of ConnectionId > OK, thanks for the explanation. I think Todd's justification for per-RPC mi yea, we could always make two proxies if we needed to. Also this isn't a public API so our decision isn't set in stone :) -- To view, visit http://gerrit.cloudera.org:8080/11681 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2 Gerrit-Change-Number: 11681 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 19:52:15 + Gerrit-HasComments: Yes
[kudu-CR] (01/05) delta store: avoid copying deleted row data in ApplyUpdates
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11856 ) Change subject: (01/05) delta_store: avoid copying deleted row data in ApplyUpdates .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11856 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6a646d0816a96e9aba486c0d0a1e6b4a7e15c144 Gerrit-Change-Number: 11856 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 19:52:00 + Gerrit-HasComments: No
[kudu-CR] [dist-test] build environment for tests in loop mode
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11905 )
Change subject: [dist-test] build environment for tests in loop mode
..
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11905/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11905/3//COMMIT_MSG@10
PS3, Line 10: dest
> nit: dist
Done
http://gerrit.cloudera.org:8080/#/c/11905/3/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/11905/3/build-support/dist_test.py@525
PS3, Line 525: tests_regex = "^" + os.path.basename(options.cmd) +
"(\.[0-9]+)?$"
> nit: mind describing what this matches?
Done
http://gerrit.cloudera.org:8080/#/c/11905/3/build-support/dist_test.py@534
PS3, Line 534: e.find("--gtest_filter") == 0 or e.find("--gtest-filter") == 0
> nit: I think you can do:
I'll punt on this because from paranoid's point of view that looks fragile: I
don't want that to react on --gtest_filter to be elsewhere but in the beginning
of the string. Basically, I don't that to trigger for flags like
'-x=--gtest_filter=*Turbo*' or alike.
http://gerrit.cloudera.org:8080/#/c/11905/3/build-support/dist_test.py@537
PS3, Line 537: e = executions[0]
: e.env["GTEST_TOTAL_SHARDS"] = 1
: e.env["GTEST_SHARD_INDEX"] = 0
> What's special about the first execution?
It's necessary to get just one out of many if it's necessary to avoid sharding.
It would be possible to take any, but it's necessary to override shard-related
environment variables.
I'll add corresponding comment.
--
To view, visit http://gerrit.cloudera.org:8080/11905
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d
Gerrit-Change-Number: 11905
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 19:38:05 +
Gerrit-HasComments: Yes
[kudu-CR] [dist-test] build environment for tests in loop mode
Hello Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11905 to look at the new patch set (#4). Change subject: [dist-test] build environment for tests in loop mode .. [dist-test] build environment for tests in loop mode This patch fixes the issue of not deploying corresponding DATA_FILES by dist-test when running a test via the loop sub-command. I used the following command to run the ts_location_assignment-itest in dist-test: KUDU_ALLOW_SLOW_TESTS=1 \ ../../build-support/dist_test.py loop -n 32 \ ./bin/ts_location_assignment-itest --stress_cpu_threads=16 Prior to this patch, all runs failed because of the absence of assign-location.py script at dist-test slaves: http://dist-test.cloudera.org/job?job_id=aserbin.1541627808.8037 With this patch, all runs succeeded: http://dist-test.cloudera.org/job?job_id=aserbin.1541627254.115588 Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d --- M build-support/dist_test.py 1 file changed, 54 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/11905/4 -- To view, visit http://gerrit.cloudera.org:8080/11905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d Gerrit-Change-Number: 11905 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong
[kudu-CR] gradle: expose testRandomSeed property
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11908 ) Change subject: gradle: expose testRandomSeed property .. Patch Set 2: Verified+1 Overriding Jenkins, DefaultSourceTest failed three times in TSAN. -- To view, visit http://gerrit.cloudera.org:8080/11908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d Gerrit-Change-Number: 11908 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 19:28:05 + Gerrit-HasComments: No
[kudu-CR] gradle: expose testRandomSeed property
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11908 ) Change subject: gradle: expose testRandomSeed property .. gradle: expose testRandomSeed property In order to read a system property in tests, it isn't enough to add a System.getProperty() call to the test; we need to expose the property in gradle so that it's properly passed through to the test. With this patch, testRandomSeed can be set using either -D or -P. Note that even with testRandomSeed overridden, the PRNG didn't produce deterministic results in TestKuduBackup.testRandomBackupAndRestore, but perhaps that's a quirk of Scala's Random class. Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d Reviewed-on: http://gerrit.cloudera.org:8080/11908 Reviewed-by: Grant Henke Tested-by: Adar Dembo --- M java/gradle/tests.gradle 1 file changed, 7 insertions(+), 0 deletions(-) Approvals: Grant Henke: Looks good to me, approved Adar Dembo: Verified -- To view, visit http://gerrit.cloudera.org:8080/11908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d Gerrit-Change-Number: 11908 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke
[kudu-CR] gradle: expose testRandomSeed property
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11908 ) Change subject: gradle: expose testRandomSeed property .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d Gerrit-Change-Number: 11908 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke
[kudu-CR] gradle: expose testRandomSeed property
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11908 ) Change subject: gradle: expose testRandomSeed property .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d Gerrit-Change-Number: 11908 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 19:19:05 + Gerrit-HasComments: No
[kudu-CR] [backup] Ensure random tests are reproducible
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11909 ) Change subject: [backup] Ensure random tests are reproducible .. [backup] Ensure random tests are reproducible Changes the use of Random in the backup tests to ensure that passing the same seed will result in the same test being run. Change-Id: I4f376ba5709ec118c4327881f7c864b23f92ac4d Reviewed-on: http://gerrit.cloudera.org:8080/11909 Reviewed-by: Adar Dembo Tested-by: Grant Henke --- M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala 1 file changed, 47 insertions(+), 41 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Grant Henke: Verified -- To view, visit http://gerrit.cloudera.org:8080/11909 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I4f376ba5709ec118c4327881f7c864b23f92ac4d Gerrit-Change-Number: 11909 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [backup] Ensure random tests are reproducible
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11909 ) Change subject: [backup] Ensure random tests are reproducible .. Patch Set 2: The TSAN build cause the create table master crash described in KUDU-2622: http://dist-test.cloudera.org/job?job_id=jenkins-slave.1541701774.20537# -- To view, visit http://gerrit.cloudera.org:8080/11909 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4f376ba5709ec118c4327881f7c864b23f92ac4d Gerrit-Change-Number: 11909 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 19:04:03 + Gerrit-HasComments: No
[kudu-CR] [backup] Ensure random tests are reproducible
Grant Henke has removed a vote on this change. Change subject: [backup] Ensure random tests are reproducible .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11909 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I4f376ba5709ec118c4327881f7c864b23f92ac4d Gerrit-Change-Number: 11909 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [backup] Ensure random tests are reproducible
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11909 ) Change subject: [backup] Ensure random tests are reproducible .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11909 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4f376ba5709ec118c4327881f7c864b23f92ac4d Gerrit-Change-Number: 11909 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 19:04:09 + Gerrit-HasComments: No
[kudu-CR] [backup] Ensure random tests are reproducible
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11909 ) Change subject: [backup] Ensure random tests are reproducible .. Patch Set 2: It uses a new instance of java.util.Random each time. -- To view, visit http://gerrit.cloudera.org:8080/11909 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4f376ba5709ec118c4327881f7c864b23f92ac4d Gerrit-Change-Number: 11909 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 18:55:54 + Gerrit-HasComments: No
[kudu-CR] [backup] Ensure random tests are reproducible
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11909 to look at the new patch set (#2). Change subject: [backup] Ensure random tests are reproducible .. [backup] Ensure random tests are reproducible Changes the use of Random in the backup tests to ensure that passing the same seed will result in the same test being run. Change-Id: I4f376ba5709ec118c4327881f7c864b23f92ac4d --- M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala 1 file changed, 47 insertions(+), 41 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/11909/2 -- To view, visit http://gerrit.cloudera.org:8080/11909 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4f376ba5709ec118c4327881f7c864b23f92ac4d Gerrit-Change-Number: 11909 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Add "network plane" as part of ConnectionId
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11681 ) Change subject: Add "network_plane" as part of ConnectionId .. Add "network_plane" as part of ConnectionId The motivation for doing so is to allow N services on the same host to be multiplexed on M different connections. For instance, a server may host multiple KRPC based services: one for control command and one for data transfer. Separating the connections between the control channel and the data channel prevents unnecessary delays of the control commands due to being stuck behind large data transfers from client to server. By default, the network_plane of a new ConnectionId is not set. A user can change it to a different value by calling Proxy::set_network_plane() on the ConnectionId. Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2 Reviewed-on: http://gerrit.cloudera.org:8080/11681 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M src/kudu/rpc/connection_id.cc M src/kudu/rpc/connection_id.h M src/kudu/rpc/messenger.h M src/kudu/rpc/proxy.cc M src/kudu/rpc/proxy.h M src/kudu/rpc/reactor.h M src/kudu/rpc/rpc-test.cc 7 files changed, 138 insertions(+), 31 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/11681 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2 Gerrit-Change-Number: 11681 Gerrit-PatchSet: 6 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [dist-test] build environment for tests in loop mode
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11905 )
Change subject: [dist-test] build environment for tests in loop mode
..
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11905/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11905/3//COMMIT_MSG@10
PS3, Line 10: dest
nit: dist
http://gerrit.cloudera.org:8080/#/c/11905/3/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/11905/3/build-support/dist_test.py@525
PS3, Line 525: tests_regex = "^" + os.path.basename(options.cmd) +
"(\.[0-9]+)?$"
nit: mind describing what this matches?
http://gerrit.cloudera.org:8080/#/c/11905/3/build-support/dist_test.py@534
PS3, Line 534: e.find("--gtest_filter") == 0 or e.find("--gtest-filter") == 0
nit: I think you can do:
[e for e in options.args if "--gtest_filter" in e or "--gtest-filter" in e]
http://gerrit.cloudera.org:8080/#/c/11905/3/build-support/dist_test.py@537
PS3, Line 537: e = executions[0]
: e.env["GTEST_TOTAL_SHARDS"] = 1
: e.env["GTEST_SHARD_INDEX"] = 0
What's special about the first execution?
--
To view, visit http://gerrit.cloudera.org:8080/11905
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d
Gerrit-Change-Number: 11905
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Comment-Date: Thu, 08 Nov 2018 18:21:44 +
Gerrit-HasComments: Yes
[kudu-CR] gradle: expose testRandomSeed property
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11908 )
Change subject: gradle: expose testRandomSeed property
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11908/1/java/gradle/tests.gradle
File java/gradle/tests.gradle:
http://gerrit.cloudera.org:8080/#/c/11908/1/java/gradle/tests.gradle@69
PS1, Line 69: if (propertyExists("testRandomSeed")) {
> This can't default to "null". It results in a NumberFormatException when tr
Done
--
To view, visit http://gerrit.cloudera.org:8080/11908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d
Gerrit-Change-Number: 11908
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 18:58:23 +
Gerrit-HasComments: Yes
[kudu-CR] gradle: expose testRandomSeed property
Hello Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11908 to look at the new patch set (#2). Change subject: gradle: expose testRandomSeed property .. gradle: expose testRandomSeed property In order to read a system property in tests, it isn't enough to add a System.getProperty() call to the test; we need to expose the property in gradle so that it's properly passed through to the test. With this patch, testRandomSeed can be set using either -D or -P. Note that even with testRandomSeed overridden, the PRNG didn't produce deterministic results in TestKuduBackup.testRandomBackupAndRestore, but perhaps that's a quirk of Scala's Random class. Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d --- M java/gradle/tests.gradle 1 file changed, 7 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/11908/2 -- To view, visit http://gerrit.cloudera.org:8080/11908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d Gerrit-Change-Number: 11908 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [backup] Ensure random tests are reproducible
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11909 ) Change subject: [backup] Ensure random tests are reproducible .. Patch Set 2: Code-Review+2 So what PRNG instance do Random.nextFoo() calls use? -- To view, visit http://gerrit.cloudera.org:8080/11909 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4f376ba5709ec118c4327881f7c864b23f92ac4d Gerrit-Change-Number: 11909 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 18:44:39 + Gerrit-HasComments: No
[kudu-CR] [dist-test] build environment for tests in loop mode
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11905 ) Change subject: [dist-test] build environment for tests in loop mode .. Patch Set 3: Verified+1 Unrelated flake in org.apache.kudu.spark.kudu.DefaultSourceTest -- To view, visit http://gerrit.cloudera.org:8080/11905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d Gerrit-Change-Number: 11905 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Comment-Date: Thu, 08 Nov 2018 17:55:18 + Gerrit-HasComments: No
[kudu-CR] [dist-test] build environment for tests in loop mode
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11905 ) Change subject: [dist-test] build environment for tests in loop mode .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d Gerrit-Change-Number: 11905 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin
[kudu-CR] [dist-test] build environment for tests in loop mode
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11905 ) Change subject: [dist-test] build environment for tests in loop mode .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11905/2/build-support/dist_test.py File build-support/dist_test.py: http://gerrit.cloudera.org:8080/#/c/11905/2/build-support/dist_test.py@528 PS2, Line 528: # The presence of the --gtest_filter flag means running some particular test : # scenarios provided by the binary. In that case it doesn't make much sense : # to shard the execution since only the shards containing the matching tests : # scenarios would produce any meaningful results. Also, it's not trivial : # to point which particular scenario is run by particular shard anyway. : # So, simply disable sharding in that case. > Nit: reword? Maybe "The presence of the --gtest_filter flag means the user Done, thanks! -- To view, visit http://gerrit.cloudera.org:8080/11905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d Gerrit-Change-Number: 11905 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 16:59:21 + Gerrit-HasComments: Yes
[kudu-CR] [dist-test] build environment for tests in loop mode
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11905 to look at the new patch set (#3). Change subject: [dist-test] build environment for tests in loop mode .. [dist-test] build environment for tests in loop mode This patch fixes the issue of not deploying corresponding DATA_FILES by dest-test when running a test via the loop sub-command. I used the following command to run the ts_location_assignment-itest in dist-test: KUDU_ALLOW_SLOW_TESTS=1 \ ../../build-support/dist_test.py loop -n 32 \ ./bin/ts_location_assignment-itest --stress_cpu_threads=16 Prior to this patch, all runs failed because of the absence of assign-location.py script at dist-test slaves: http://dist-test.cloudera.org/job?job_id=aserbin.1541627808.8037 With this patch, all runs succeeded: http://dist-test.cloudera.org/job?job_id=aserbin.1541627254.115588 Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d --- M build-support/dist_test.py 1 file changed, 46 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/11905/3 -- To view, visit http://gerrit.cloudera.org:8080/11905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d Gerrit-Change-Number: 11905 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] gradle: expose testRandomSeed property
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/11908 )
Change subject: gradle: expose testRandomSeed property
..
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11908/1/java/gradle/tests.gradle
File java/gradle/tests.gradle:
http://gerrit.cloudera.org:8080/#/c/11908/1/java/gradle/tests.gradle@69
PS1, Line 69: systemProperty "testRandomSeed",
propertyWithDefault("testRandomSeed", null)
This can't default to "null". It results in a NumberFormatException when trying
to translate the string to an integer.
Instead I think using `if (propertyExists("testRandomSeed"))` should work.
--
To view, visit http://gerrit.cloudera.org:8080/11908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d
Gerrit-Change-Number: 11908
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 15:23:05 +
Gerrit-HasComments: Yes
[kudu-CR] [cmake] no empty KUDU DATA FILES ctest env variables
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11907 ) Change subject: [cmake] no empty KUDU_DATA_FILES ctest env variables .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11907 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb Gerrit-Change-Number: 11907 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 08:32:46 + Gerrit-HasComments: No
[kudu-CR] [backup] Ensure random tests are reproducible
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11909 Change subject: [backup] Ensure random tests are reproducible .. [backup] Ensure random tests are reproducible Changes the use of Random in the backup tests to ensure that passing the same seed will result in the same test being run. Change-Id: I4f376ba5709ec118c4327881f7c864b23f92ac4d --- M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala 1 file changed, 47 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/11909/1 -- To view, visit http://gerrit.cloudera.org:8080/11909 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I4f376ba5709ec118c4327881f7c864b23f92ac4d Gerrit-Change-Number: 11909 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] run-test: don't report leaks that don't fail test
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 ) Change subject: run-test: don't report leaks that don't fail test .. Patch Set 7: Code-Review+2 Carrying forward Adar's +2 -- To view, visit http://gerrit.cloudera.org:8080/11886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 Gerrit-Change-Number: 11886 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 03:10:20 + Gerrit-HasComments: No
[kudu-CR] Add "network plane" as part of ConnectionId
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/11681 )
Change subject: Add "network_plane" as part of ConnectionId
..
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7
PS1, Line 7: Add "service_name" as part of ConnectionId
> I'm curious to hear your thinking on, when implementing option 1, you did i
Specifying it per service seems a bit rigid as it hardcodes the data plane for
all calls in the entire service.
Specifying it per-RPC is vs per-Proxy can be functionally equivalent. After
all, a proxy is needed to make a RPC call. One can have multiple proxies with
different data planes and pick the right one for a given call. It's also more
convenient as the ConnectionId is owned by the Proxy (as the code stands now).
Piping the "network plane" through per-RPC seems slightly more complicated.
http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc
File src/kudu/rpc/connection_id.cc:
http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc@50
PS4, Line 50: }
> Nit: we're using std::string here; don't need std:: prefix.
Done
http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc@62
PS4, Line 62: user_credentials_.ToString());
> Maybe omit the plane altogether if it's the default one?
Done
http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h
File src/kudu/rpc/proxy.h:
http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@54
PS4, Line 54: r after a
> Nit: "so that a higher"
Done
http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@55
PS4, Line 55: arted will caus
> Nit: "a control channel"
Done
http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@118
PS4, Line 118: mutable Atomic32 is_started_;
> Could we pass by copy here and std::move into the underlying ConnectionId?
Done; Did the same for set_user_credentials().
http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@696
PS4, Line 696: req, &resp, &controller));
> There's a mix of ASSERT and EXPECT below; curious why you aren't just using
Just copied and pasted that pattern from TestCredentialsPolicy. I don't see any
reason for such mixed use. Converted everything to ASSERT instead.
http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@697
PS4, Line 697: }
> Test num_{client,server}_connections here too?
Done
http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@698
PS4, Line 698:
: // Test that the RpcSidecar transfers the expected messages.
: TEST_P(TestRpc, TestRpcSidecar) {
: // Set up server.
: Sockaddr server_addr;
: bool enable_ssl = GetParam();
: ASSERT_OK(StartTestServer(&server_addr, enable_s
> This is repeated below; perhaps define a 2-arg lambda to encapsulate the wh
These verification logic seem to be repeated across multiple tests at various
places. I agree that we can encapsulate these verification logic into a
function or lambda. However, the downside seems to be that it may be harder to
identify the failures. As the code stands now, the failure can be easily
identified by line number.
--
To view, visit http://gerrit.cloudera.org:8080/11681
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Sailesh Mukil
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Thu, 08 Nov 2018 00:22:53 +
Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11886 )
Change subject: run-test: don't report leaks that don't fail test
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11886/4/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:
http://gerrit.cloudera.org:8080/#/c/11886/4/src/kudu/server/generic_service.cc@183
PS4, Line 183: if (!has_leaks) {
> May want to make this bigger so it stands out from the report. And maybe WA
Done
--
To view, visit http://gerrit.cloudera.org:8080/11886
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Thu, 08 Nov 2018 00:48:19 +
Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11886 to look at the new patch set (#7). Change subject: run-test: don't report leaks that don't fail test .. run-test: don't report leaks that don't fail test LeakSanitizer will report a leak when allocating a string in SuperviseThread. It's unclear why this is the case, but upon inspecting the code, it seems like a false positive. The stack trace is as follows: = ==93677==ERROR: LeakSanitizer: detected memory leaks Direct leak of 58 byte(s) in 1 object(s) allocated from: #0 0x5318c8 in operator new(unsigned long) /data/8/awong/kudu/thirdparty/src/llvm-6.0.0.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:92 #1 0x3ae3a9c3c8 in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator const&) (/usr/lib64/libstdc++.so.6+0x3ae3a9c3c8) #2 0x3ae3a9d19a in std::string::_Rep::_M_clone(std::allocator const&, unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d19a) #3 0x3ae3a9d5eb in std::string::reserve(unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d5eb) #4 0x3ae3a9d770 in std::string::append(unsigned long, char) (/usr/lib64/libstdc++.so.6+0x3ae3a9d770) #5 0x7f518f799c12 in strings::SubstituteAndAppend(std::string*, StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.cc:110:3 #6 0x536e76 in strings::Substitute(StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.h:188:3 #7 0x7f5190590860 in kudu::Thread::SuperviseThread(void*) ../src/kudu/util/thread.cc:607:17 #8 0x3ae0e079d0 in start_thread (/lib64/libpthread.so.0+0x3ae0e079d0) #9 0x3ae0ae88fc in clone (/lib64/libc.so.6+0x3ae0ae88fc) This appears to be affecting a number tests, but generally only lines #0 and #1 are present in the logs, making them difficult to debug (a developer would have to rerun the test with specific ASAN_OPTIONS to unwind the stacktrace more). Namely, exactly_once_writes-itest (KUDU-2517), kudu-admin-test (KUDU-2583), and rebalancer-tool-test (untracked via Jira) all show the top of the above stack trace, and based on the full stack trace, it seems these are all false positives. The presence of issues like https://github.com/google/sanitizers/issues/757 confirms that LeakSanitizer can report false positives in workloads with high concurrency. Generally, the test binary will return an error in the face of real leaks, but in tests like the ones mentioned, the test may log messages reporting leaks, but not actually return an error because the "leak" was transient (e.g. see GenericServiceImpl::CheckLeaks). We currently inject errors into JUnit XML report if any leaks are reported, even for false positives, since the leak messages still find their way into the logs. This patch updates this to only inject these errors if the test also returned an error. For clarity, I also threw in a log statement to GenericServiceImpl::CheckLeaks denoting false positives. Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 --- M build-support/run-test.sh M src/kudu/server/generic_service.cc 2 files changed, 15 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/11886/7 -- To view, visit http://gerrit.cloudera.org:8080/11886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 Gerrit-Change-Number: 11886 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [cmake] no empty KUDU DATA FILES ctest env variables
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11907 )
Change subject: [cmake] no empty KUDU_DATA_FILES ctest env variables
..
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11907/3/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11907/3/CMakeLists.txt@877
PS3, Line 877: if (NOT "${DATA_FILES_LIST}" STREQUAL "")
Does this work?
if (NOT DATA_FILES_LIST)
--
To view, visit http://gerrit.cloudera.org:8080/11907
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb
Gerrit-Change-Number: 11907
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 00:34:41 +
Gerrit-HasComments: Yes
[kudu-CR] [dist-test] build environment for tests in loop mode
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11905 ) Change subject: [dist-test] build environment for tests in loop mode .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11905/2/build-support/dist_test.py File build-support/dist_test.py: http://gerrit.cloudera.org:8080/#/c/11905/2/build-support/dist_test.py@528 PS2, Line 528: # The presence of the --gtest_filter flag means running some particular test : # scenarios provided by the binary. In that case it doesn't make much sense : # to shard the execution since only the shards containing the matching tests : # scenarios would produce any meaningful results. Also, it's not trivial : # to point which particular scenario is run by particular shard anyway. : # So, simply disable sharding in that case. Nit: reword? Maybe "The presence of the --gtest_filter flag means the user is interested in a particular subset of tests provided by the binary. In that case it doesn't make sense to shard the execution since only the shards containing the matching tests would produce interesting results, while the rest would yield noise. To avoid this, we disable sharding in this case." -- To view, visit http://gerrit.cloudera.org:8080/11905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d Gerrit-Change-Number: 11905 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 08:32:27 + Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 ) Change subject: run-test: don't report leaks that don't fail test .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/11886/5/src/kudu/server/generic_service.cc File src/kudu/server/generic_service.cc: http://gerrit.cloudera.org:8080/#/c/11886/5/src/kudu/server/generic_service.cc@52 PS5, Line 52: > warning: using decl 'Substitute' is unused [misc-unused-using-decls] Done -- To view, visit http://gerrit.cloudera.org:8080/11886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 Gerrit-Change-Number: 11886 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 01:10:17 + Gerrit-HasComments: Yes
[kudu-CR] Add "network plane" as part of ConnectionId
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 ) Change subject: Add "network_plane" as part of ConnectionId .. Patch Set 5: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7 PS1, Line 7: Add "network_plane" as part of ConnectionId > Specifying it per service seems a bit rigid as it hardcodes the data plane OK, thanks for the explanation. I think Todd's justification for per-RPC might hinge on the fact that, within Kudu, we generally use one proxy for every remote+service pair. So if the service plane is a property of the proxy, we'd need to establish multiple (probably just two) proxies for each remote+service pair. Anyway I don't have a strong feeling about this, and judging by Todd's earlier suggestions, neither does he. -- To view, visit http://gerrit.cloudera.org:8080/11681 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2 Gerrit-Change-Number: 11681 Gerrit-PatchSet: 5 Gerrit-Owner: Michael Ho Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 00:33:46 + Gerrit-HasComments: Yes
[kudu-CR] [cmake] don't add empty KUDU DATA FILES env variable
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11907 Change subject: [cmake] don't add empty KUDU_DATA_FILES env variable .. [cmake] don't add empty KUDU_DATA_FILES env variable This patch introduces an update on ADD_KUDU_TEST() custom function: if DATA_FILES is not specified for a test, don't add KUDU_DATA_FILES variable into its ctest environment. Prior to this patch, ctest environment of every test binary would contain KUDU_DATA_FILES variable. It was set to a value reflecting DATA_FILES variable defined for a test, or to an empty string otherwise. Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb --- M CMakeLists.txt 1 file changed, 8 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11907/1 -- To view, visit http://gerrit.cloudera.org:8080/11907 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb Gerrit-Change-Number: 11907 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] run-test: don't report leaks that don't fail test
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 ) Change subject: run-test: don't report leaks that don't fail test .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 Gerrit-Change-Number: 11886 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 00:48:38 + Gerrit-HasComments: No
[kudu-CR] run-test: don't report leaks that don't fail test
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11886 ) Change subject: run-test: don't report leaks that don't fail test .. run-test: don't report leaks that don't fail test LeakSanitizer will report a leak when allocating a string in SuperviseThread. It's unclear why this is the case, but upon inspecting the code, it seems like a false positive. The stack trace is as follows: = ==93677==ERROR: LeakSanitizer: detected memory leaks Direct leak of 58 byte(s) in 1 object(s) allocated from: #0 0x5318c8 in operator new(unsigned long) /data/8/awong/kudu/thirdparty/src/llvm-6.0.0.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:92 #1 0x3ae3a9c3c8 in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator const&) (/usr/lib64/libstdc++.so.6+0x3ae3a9c3c8) #2 0x3ae3a9d19a in std::string::_Rep::_M_clone(std::allocator const&, unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d19a) #3 0x3ae3a9d5eb in std::string::reserve(unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d5eb) #4 0x3ae3a9d770 in std::string::append(unsigned long, char) (/usr/lib64/libstdc++.so.6+0x3ae3a9d770) #5 0x7f518f799c12 in strings::SubstituteAndAppend(std::string*, StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.cc:110:3 #6 0x536e76 in strings::Substitute(StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.h:188:3 #7 0x7f5190590860 in kudu::Thread::SuperviseThread(void*) ../src/kudu/util/thread.cc:607:17 #8 0x3ae0e079d0 in start_thread (/lib64/libpthread.so.0+0x3ae0e079d0) #9 0x3ae0ae88fc in clone (/lib64/libc.so.6+0x3ae0ae88fc) This appears to be affecting a number tests, but generally only lines #0 and #1 are present in the logs, making them difficult to debug (a developer would have to rerun the test with specific ASAN_OPTIONS to unwind the stacktrace more). Namely, exactly_once_writes-itest (KUDU-2517), kudu-admin-test (KUDU-2583), and rebalancer-tool-test (untracked via Jira) all show the top of the above stack trace, and based on the full stack trace, it seems these are all false positives. The presence of issues like https://github.com/google/sanitizers/issues/757 confirms that LeakSanitizer can report false positives in workloads with high concurrency. Generally, the test binary will return an error in the face of real leaks, but in tests like the ones mentioned, the test may log messages reporting leaks, but not actually return an error because the "leak" was transient (e.g. see GenericServiceImpl::CheckLeaks). We currently inject errors into JUnit XML report if any leaks are reported, even for false positives, since the leak messages still find their way into the logs. This patch updates this to only inject these errors if the test also returned an error. For clarity, I also threw in a log statement to GenericServiceImpl::CheckLeaks denoting false positives. Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 Reviewed-on: http://gerrit.cloudera.org:8080/11886 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M build-support/run-test.sh M src/kudu/server/generic_service.cc 2 files changed, 15 insertions(+), 5 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 Gerrit-Change-Number: 11886 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] run-test: don't report leaks that don't fail test
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 ) Change subject: run-test: don't report leaks that don't fail test .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/11886/5/src/kudu/server/generic_service.cc File src/kudu/server/generic_service.cc: http://gerrit.cloudera.org:8080/#/c/11886/5/src/kudu/server/generic_service.cc@52 PS5, Line 52: > Odd suggestion, seeing as you were clearly using it. I think it's mad because the `Substitute` is in a `ifndef` block. -- To view, visit http://gerrit.cloudera.org:8080/11886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 Gerrit-Change-Number: 11886 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 01:18:16 + Gerrit-HasComments: Yes
[kudu-CR] [dist-test] build environment for tests in loop mode
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11905 ) Change subject: [dist-test] build environment for tests in loop mode .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/11905/1/build-support/dist_test.py File build-support/dist_test.py: http://gerrit.cloudera.org:8080/#/c/11905/1/build-support/dist_test.py@167 PS1, Line 167: Return an array of TestExecution objects. > Please doc tests_regex and extra_args. Done http://gerrit.cloudera.org:8080/#/c/11905/1/build-support/dist_test.py@170 PS1, Line 170: ctest_argv = [ctest_bin, "-V", "-N", "-LE", "no_dist_test"] > Yep, I'll take a look. http://gerrit.cloudera.org:8080/11907 http://gerrit.cloudera.org:8080/#/c/11905/1/build-support/dist_test.py@517 PS1, Line 517: # Build the list of executions corresponding to the test. It's better to do > Yes, this will automatically shard the test. Isn't it good? In offline discussion Todd suggested to use the presence of the --gtest_filter option to decide whether to shard or not. -- To view, visit http://gerrit.cloudera.org:8080/11905 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d Gerrit-Change-Number: 11905 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 02:15:56 + Gerrit-HasComments: Yes
[kudu-CR] [cmake] no empty KUDU DATA FILES ctest env variables
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11907 ) Change subject: [cmake] no empty KUDU_DATA_FILES ctest env variables .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11907/2/CMakeLists.txt File CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/11907/2/CMakeLists.txt@876 PS2, Line 876: If the DATA_FILES_LIST is : # empty, do not add the KUDU_DATA_FILES environment variable. FWIW, I think this is obvious from the code. -- To view, visit http://gerrit.cloudera.org:8080/11907 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb Gerrit-Change-Number: 11907 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 08 Nov 2018 00:27:37 + Gerrit-HasComments: Yes
[kudu-CR] run-test: don't report leaks that don't fail test
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11886 to look at the new patch set (#6). Change subject: run-test: don't report leaks that don't fail test .. run-test: don't report leaks that don't fail test LeakSanitizer will report a leak when allocating a string in SuperviseThread. It's unclear why this is the case, but upon inspecting the code, it seems like a false positive. The stack trace is as follows: = ==93677==ERROR: LeakSanitizer: detected memory leaks Direct leak of 58 byte(s) in 1 object(s) allocated from: #0 0x5318c8 in operator new(unsigned long) /data/8/awong/kudu/thirdparty/src/llvm-6.0.0.src/projects/compiler-rt/lib/asan/asan_new_delete.cc:92 #1 0x3ae3a9c3c8 in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator const&) (/usr/lib64/libstdc++.so.6+0x3ae3a9c3c8) #2 0x3ae3a9d19a in std::string::_Rep::_M_clone(std::allocator const&, unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d19a) #3 0x3ae3a9d5eb in std::string::reserve(unsigned long) (/usr/lib64/libstdc++.so.6+0x3ae3a9d5eb) #4 0x3ae3a9d770 in std::string::append(unsigned long, char) (/usr/lib64/libstdc++.so.6+0x3ae3a9d770) #5 0x7f518f799c12 in strings::SubstituteAndAppend(std::string*, StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.cc:110:3 #6 0x536e76 in strings::Substitute(StringPiece, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&, strings::internal::SubstituteArg const&) ../src/kudu/gutil/strings/substitute.h:188:3 #7 0x7f5190590860 in kudu::Thread::SuperviseThread(void*) ../src/kudu/util/thread.cc:607:17 #8 0x3ae0e079d0 in start_thread (/lib64/libpthread.so.0+0x3ae0e079d0) #9 0x3ae0ae88fc in clone (/lib64/libc.so.6+0x3ae0ae88fc) This appears to be affecting a number tests, but generally only lines #0 and #1 are present in the logs, making them difficult to debug (a developer would have to rerun the test with specific ASAN_OPTIONS to unwind the stacktrace more). Namely, exactly_once_writes-itest (KUDU-2517), kudu-admin-test (KUDU-2583), and rebalancer-tool-test (untracked via Jira) all show the top of the above stack trace, and based on the full stack trace, it seems these are all false positives. The presence of issues like https://github.com/google/sanitizers/issues/757 confirms that LeakSanitizer can report false positives in workloads with high concurrency. Generally, the test binary will return an error in the face of real leaks, but in tests like the ones mentioned, the test may log messages reporting leaks, but not actually return an error because the "leak" was transient (e.g. see GenericServiceImpl::CheckLeaks). We currently inject errors into JUnit XML report if any leaks are reported, even for false positives, since the leak messages still find their way into the logs. This patch updates this to only inject these errors if the test also returned an error. For clarity, I also threw in a log statement to GenericServiceImpl::CheckLeaks denoting false positives. Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 --- M build-support/run-test.sh M src/kudu/server/generic_service.cc 2 files changed, 15 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/86/11886/6 -- To view, visit http://gerrit.cloudera.org:8080/11886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 Gerrit-Change-Number: 11886 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] run-test: don't report leaks that don't fail test
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11886 ) Change subject: run-test: don't report leaks that don't fail test .. Patch Set 6: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11886/5/src/kudu/server/generic_service.cc File src/kudu/server/generic_service.cc: http://gerrit.cloudera.org:8080/#/c/11886/5/src/kudu/server/generic_service.cc@52 PS5, Line 52: > Done Odd suggestion, seeing as you were clearly using it. -- To view, visit http://gerrit.cloudera.org:8080/11886 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647 Gerrit-Change-Number: 11886 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 08 Nov 2018 01:17:08 + Gerrit-HasComments: Yes
