[kudu-CR] (02/05) delta store: convert DeltaIterator::PrepareBatch flags into bitfield
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/11857 to look at the new patch set (#2). 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/2 -- 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: 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
[kudu-CR] (01/05) delta store: avoid copying deleted row data in ApplyUpdates
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/11856 to look at the new patch set (#2). 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 --- 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(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11856/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: newpatchset 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
[kudu-CR] (05/05) delta applier: support for diff scan style iteration
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/11860 to look at the new patch set (#2). Change subject: (05/05) delta_applier: support for diff scan style iteration .. (05/05) delta_applier: support for diff scan style iteration This patch incorporates all of the previous work to provide diff scan support for an entire DiskRowSet. There's a new "fuzzy" test in diskrowset-test that exercises this functionality. Change-Id: I895350a3c7728df88a87a36b904ffdea4f3f6b7a --- M src/kudu/tablet/delta_applier.cc M src/kudu/tablet/delta_applier.h M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/diskrowset-test.cc 5 files changed, 287 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/11860/2 -- To view, visit http://gerrit.cloudera.org:8080/11860 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I895350a3c7728df88a87a36b904ffdea4f3f6b7a Gerrit-Change-Number: 11860 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
[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 (#2). 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/2 -- 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: 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
[kudu-CR] (04/05) delta store: support iteration with is deleted virtual column
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/11859 to look at the new patch set (#2). Change subject: (04/05) delta_store: support iteration with is_deleted virtual column .. (04/05) delta_store: support iteration with is_deleted virtual column This patch adds support to delta stores (i.e. DMS and delta files) for iterating with an IS_DELETED virtual column. A diff scan will include IS_DELETED in the projection so that clients can tell if a row was deleted in the requested time range. Change-Id: Ibafcae9f9f82a6c2d9a8afedb8fbf07d6e3069ec --- M src/kudu/common/schema.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/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/tablet-test-util.h 7 files changed, 99 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11859/2 -- To view, visit http://gerrit.cloudera.org:8080/11859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ibafcae9f9f82a6c2d9a8afedb8fbf07d6e3069ec Gerrit-Change-Number: 11859 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
[kudu-CR] deltamemstore: support iteration with snap to exclude
Adar Dembo has abandoned this change. ( http://gerrit.cloudera.org:8080/11029 ) Change subject: deltamemstore: support iteration with snap_to_exclude .. Abandoned This ended up being more complicated and I wrote a new series of changes to do it. -- To view, visit http://gerrit.cloudera.org:8080/11029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47 Gerrit-Change-Number: 11029 Gerrit-PatchSet: 11 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] deltas: add SelectUpdates iterator method
Adar Dembo has abandoned this change. ( http://gerrit.cloudera.org:8080/11137 ) Change subject: deltas: add SelectUpdates iterator method .. Abandoned Ended up merging this into another change, because it made more sense to do so. -- To view, visit http://gerrit.cloudera.org:8080/11137 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I5dda1787b8dfa64bc86800b8883499929eef9fef Gerrit-Change-Number: 11137 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] WIP: DeltaApplier changes
Adar Dembo has abandoned this change. ( http://gerrit.cloudera.org:8080/11139 ) Change subject: WIP: DeltaApplier changes .. Abandoned Revamped this in a new change. -- To view, visit http://gerrit.cloudera.org:8080/11139 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I787932ef49d2207e00a4e30a6cced9a9c0f7eb89 Gerrit-Change-Number: 11139 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] WIP: DeltaFileIterator snap to exclude
Adar Dembo has abandoned this change. ( http://gerrit.cloudera.org:8080/11138 ) Change subject: WIP: DeltaFileIterator snap_to_exclude .. Abandoned Revamped this in a new change. -- To view, visit http://gerrit.cloudera.org:8080/11138 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I5fa5e4c689d3d8a5a56f8bdd91d2047e1f2f85a6 Gerrit-Change-Number: 11138 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] delta store: support iteration with snap to exclude
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11858 to review the following change. Change subject: delta_store: support iteration with snap_to_exclude .. 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/1 -- 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: newchange Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c Gerrit-Change-Number: 11858 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] delta store: convert DeltaIterator::PrepareBatch flags into bitfield
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11857 to review the following change. Change subject: delta_store: convert DeltaIterator::PrepareBatch flags into bitfield .. 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/1 -- 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: newchange Gerrit-Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a Gerrit-Change-Number: 11857 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] delta store: avoid copying deleted row data in ApplyUpdates
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11856 to review the following change. Change subject: delta_store: avoid copying deleted row data in ApplyUpdates .. 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 --- 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(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/11856/1 -- 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: newchange Gerrit-Change-Id: I6a646d0816a96e9aba486c0d0a1e6b4a7e15c144 Gerrit-Change-Number: 11856 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] delta store: support iteration with is deleted virtual column
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11859 to review the following change. Change subject: delta_store: support iteration with is_deleted virtual column .. delta_store: support iteration with is_deleted virtual column This patch adds support to delta stores (i.e. DMS and delta files) for iterating with an IS_DELETED virtual column. A diff scan will include IS_DELETED in the projection so that clients can tell if a row was deleted in the requested time range. Change-Id: Ibafcae9f9f82a6c2d9a8afedb8fbf07d6e3069ec --- M src/kudu/common/schema.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/memrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/tablet-test-util.h 7 files changed, 99 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11859/1 -- To view, visit http://gerrit.cloudera.org:8080/11859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ibafcae9f9f82a6c2d9a8afedb8fbf07d6e3069ec Gerrit-Change-Number: 11859 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] delta applier: support for diff scan style iteration
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11860 to review the following change. Change subject: delta_applier: support for diff scan style iteration .. delta_applier: support for diff scan style iteration This patch incorporates all of the previous work to provide diff scan support for an entire DiskRowSet. There's a new "fuzzy" test in diskrowset-test that exercises this functionality. Change-Id: I895350a3c7728df88a87a36b904ffdea4f3f6b7a --- M src/kudu/tablet/delta_applier.cc M src/kudu/tablet/delta_applier.h M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/delta_tracker.h M src/kudu/tablet/diskrowset-test.cc 5 files changed, 287 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/11860/1 -- To view, visit http://gerrit.cloudera.org:8080/11860 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I895350a3c7728df88a87a36b904ffdea4f3f6b7a Gerrit-Change-Number: 11860 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [sentry] add AuthzProvider
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
..
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/sentry_authz_provider.cc@246
PS11, Line 246:
: // Validates the sentry_service_rpc_addresses gflag.
: bool SentryAuthzProvider::ValidateAddresses(const char*
flag_name,
: const string&
addresses) {
: vector host_ports;
: Status s = HostPort::ParseStringsWithScheme(addresses,
SentryClient::kDefaultSentryPort,
: _ports);
: if (!s.ok()) {
: LOG(ERROR) << "invalid flag " << flag_name << ": " <<
s.ToString();
: }
: return s.ok();
: }
Now that this is tested elsewhere, it doesn't need to be part of the class.
Stick it in an anonymous namespace?
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 04:48:12 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Andrew Wong has uploaded a new patch set (#6) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/6348 ) Change subject: KUDU-1918 Prevent hijacking of scanner IDs .. KUDU-1918 Prevent hijacking of scanner IDs This makes the scanner remember its RemoteUser, and ensures that when continuing a scan, the new requestor matches the original requestor. This prevents one user from somehow obtaining a scanner ID from another and then "hijacking" the in-progress scan. This restricts scans, checksum scans, and keep-alive requests. Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4 --- M src/kudu/client/client-test.cc M src/kudu/client/client.h M src/kudu/client/scanner-internal.cc M src/kudu/client/scanner-internal.h M src/kudu/tserver/scanners-test.cc M src/kudu/tserver/scanners.cc M src/kudu/tserver/scanners.h M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tserver.proto M src/kudu/tserver/tserver_path_handlers.cc 13 files changed, 265 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6348/6 -- To view, visit http://gerrit.cloudera.org:8080/6348 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4 Gerrit-Change-Number: 6348 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 ) Change subject: KUDU-1918 Prevent hijacking of scanner IDs .. Patch Set 5: Fixed IWYU -- To view, visit http://gerrit.cloudera.org:8080/6348 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4 Gerrit-Change-Number: 6348 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 02 Nov 2018 02:24:11 + Gerrit-HasComments: No
[kudu-CR] [sentry] add AuthzProvider
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 ) Change subject: [sentry] add AuthzProvider .. Patch Set 11: Code-Review+1 (2 comments) Sorry, a couple small nits and LGTM. http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/authz_provider.h File src/kudu/master/authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/authz_provider.h@63 PS11, Line 63: // Checks if retrieving table's metadata is authorized for the given user. nit: "metadata" seems like it might be a Sentry construct, which is fine, but I think it'd help to flesh this out a bit more. IMO it's not obvious from just reading this how and where this should be used. WDYT? Something like: // Checks if retrieving metadata about the table is authorized for the given user. // E.g. when checking for table presence or locations, checking the status of DDL operations, etc. http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/11/src/kudu/master/sentry_authz_provider.h@39 PS11, Line 39: : // An implementation of AuthzProvider, which connects to the Sentry Service : // for authorization metadata and allow or deny the actions performed by : // users based on the metadata. nit: "An implementation of AuthzProvider that connects to Apache Sentry for authorization metadata and allows or denies actions performed by users based on the metadata." -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 02 Nov 2018 02:09:45 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 ) Change subject: KUDU-1918 Prevent hijacking of scanner IDs .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc@3400 PS3, Line 3400: // bytes should be read by the BM if storing keys in the rowset metadata). > On second thought, this could probably use a scanner-side test. Done -- To view, visit http://gerrit.cloudera.org:8080/6348 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4 Gerrit-Change-Number: 6348 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 02 Nov 2018 01:35:31 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Andrew Wong has uploaded a new patch set (#5) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/6348 ) Change subject: KUDU-1918 Prevent hijacking of scanner IDs .. KUDU-1918 Prevent hijacking of scanner IDs This makes the scanner remember its RemoteUser, and ensures that when continuing a scan, the new requestor matches the original requestor. This prevents one user from somehow obtaining a scanner ID from another and then "hijacking" the in-progress scan. Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4 --- M src/kudu/client/client-test.cc M src/kudu/client/client.h M src/kudu/client/scanner-internal.cc M src/kudu/client/scanner-internal.h M src/kudu/tserver/scanners-test.cc M src/kudu/tserver/scanners.cc M src/kudu/tserver/scanners.h M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tserver.proto M src/kudu/tserver/tserver_path_handlers.cc 13 files changed, 262 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6348/5 -- To view, visit http://gerrit.cloudera.org:8080/6348 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4 Gerrit-Change-Number: 6348 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
..
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@117
PS10, Line 117:
: namespace kudu {
:
> Makes sense.
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 02 Nov 2018 00:42:41 +
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 ) Change subject: [sentry] add AuthzProvider .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider-test.cc@270 PS10, Line 270: ASSERT_OK(sentry_authz_provider_->AuthorizeGetTableMetadata( : "db.table", kTestUser)); : }); > nit: aesthetically, I generally prefer, but feel free to ignore if you like Done -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 02 Nov 2018 00:42:24 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11659 to look at the new patch set (#11). Change subject: [sentry] add AuthzProvider .. [sentry] add AuthzProvider This commit adds a high-level abstraction which handles authorizations on Kudu operations, called AuthzProvider. It has a default implementation which always allow any operations for any users, and a SentryAuthzProvider implementation which connects to the Sentry service for authorization metadata. AuthzProvider, along with its implementations, is placed in the master module. The idea is to decouple it from Sentry since in the future, other authorization implementations might be introduced. A follow-up commit will integrate the AuthzProvider into CatalogManager to perform authorization checks on Master RPCs. Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 --- M src/kudu/hms/hms_catalog.cc M src/kudu/master/CMakeLists.txt A src/kudu/master/authz_provider.h A src/kudu/master/default_authz_provider.h A src/kudu/master/sentry_authz_provider-test.cc A src/kudu/master/sentry_authz_provider.cc A src/kudu/master/sentry_authz_provider.h A src/kudu/sentry/sentry-test-base.h M src/kudu/sentry/sentry_action-test.cc M src/kudu/sentry/sentry_action.cc M src/kudu/sentry/sentry_action.h M src/kudu/sentry/sentry_client-test.cc M src/kudu/sentry/sentry_client.cc M src/kudu/sentry/sentry_client.h 14 files changed, 994 insertions(+), 117 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/11 -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 ) Change subject: [sentry] add AuthzProvider .. Patch Set 10: (2 comments) http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@84 PS10, Line 84: 1 > nit: does the default setting of 1 seem good enough for the production use? I think one time retry is general enough to avoid to end the connection too early. Or you have other suggestions? I do not quite follow the second question. Retry count and timeout settings are related but it doesn't mean the former is ignored. http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@117 PS10, Line 117: TAG_FLAG(sentry_service_max_message_size_bytes, advanced); : TAG_FLAG(sentry_service_max_message_size_bytes, experimental); : TAG_FLAG(sentry_service_max_message_size_bytes, runtime); > These only get set once when the AuthzProvider starts up; should the be non Makes sense. -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 02 Nov 2018 00:33:19 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 ) Change subject: [sentry] add AuthzProvider .. Patch Set 10: -Code-Review (2 comments) http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@84 PS10, Line 84: 1 nit: does the default setting of 1 seem good enough for the production use? Another question: is it possible to set this to some 'do not care' value but rely purely on the timeout settings instead? http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@117 PS10, Line 117: TAG_FLAG(sentry_service_max_message_size_bytes, advanced); : TAG_FLAG(sentry_service_max_message_size_bytes, experimental); : TAG_FLAG(sentry_service_max_message_size_bytes, runtime); > These only get set once when the AuthzProvider starts up; should the be non +1 Indeed, all these flags for the underlying thrift client do not qualify for runtime ones as it seems from the implementation. -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Nov 2018 23:00:56 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/6348 ) Change subject: KUDU-1918 Prevent hijacking of scanner IDs .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc@3400 PS3, Line 3400: // bytes should be read by the BM if storing keys in the rowset metadata). > I'll keep this in mind for further authz testing. On second thought, this could probably use a scanner-side test. -- To view, visit http://gerrit.cloudera.org:8080/6348 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4 Gerrit-Change-Number: 6348 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Nov 2018 22:58:05 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
..
Patch Set 10: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider-test.cc@270
PS10, Line 270:
ASSERT_OK(sentry_authz_provider_->AuthorizeGetTableMetadata(
: "db.table", kTestUser));
: });
nit: aesthetically, I generally prefer, but feel free to ignore if you like
this more (elsewhere too):
ASSERT_EVENTUALLY([&] {
ASSERT_OK(sentry_authz_provider_->AuthorizeGetTabletMetadata(
"db.table", kTestUser));
});
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/10/src/kudu/master/sentry_authz_provider.cc@117
PS10, Line 117: TAG_FLAG(sentry_service_max_message_size_bytes, advanced);
: TAG_FLAG(sentry_service_max_message_size_bytes, experimental);
: TAG_FLAG(sentry_service_max_message_size_bytes, runtime);
These only get set once when the AuthzProvider starts up; should the be
non-runtime then?
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 22:37:46 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/6348 )
Change subject: KUDU-1918 Prevent hijacking of scanner IDs
..
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test-base.h
File src/kudu/tserver/tablet_server-test-base.h:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test-base.h@125
PS3, Line 125: Status FillNewScanRequest(ReadMode read_mode,
NewScanRequestPB* scan) const;
> Could it be a static method (or at least const)?
Done, const since it's using `schema_`
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc@3400
PS3, Line 3400: // bytes should be read by the BM if storing keys in the
rowset metadata).
> Is it worth adding a higher-level test to make sure this works as expected?
I'll keep this in mind for further authz testing.
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_server-test.cc@3407
PS3, Line 3407: uy");
> Does it make sense to add a scenario to verify that without user_credential
Done
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_service.cc@1292
PS3, Line 1292: void TabletServiceImpl::ScannerKeepAlive(const
ScannerKeepAliveRequestPB *req,
> I'd suggest going ahead and doing this as a part of this change, unless it'
Done
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tablet_service.cc@2066
PS3, Line 2066:
);
> Why not populate *error_code with something more detailed? Is UNKNOWN_ERROR
Done
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tserver_path_handlers.cc
File src/kudu/tserver/tserver_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/6348/3/src/kudu/tserver/tserver_path_handlers.cc@540
PS3, Line 540: json->Set("requestor", scan.remote_user.username());
> Also add an underscore, I think that's the prevailing style for our JSON ke
Hrm, I think I'll keep requestor, even if it isn't exactly consistent; in the
user-facing context of a scan, it's clear what a Requestor is.
--
To view, visit http://gerrit.cloudera.org:8080/6348
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4
Gerrit-Change-Number: 6348
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 22:17:11 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1918 Prevent hijacking of scanner IDs
Andrew Wong has uploaded a new patch set (#4) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/6348 ) Change subject: KUDU-1918 Prevent hijacking of scanner IDs .. KUDU-1918 Prevent hijacking of scanner IDs This makes the scanner remember its RemoteUser, and ensures that when continuing a scan, the new requestor matches the original requestor. This prevents one user from somehow obtaining a scanner ID from another and then "hijacking" the in-progress scan. Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4 --- M src/kudu/client/scanner-internal.cc M src/kudu/client/scanner-internal.h M src/kudu/tserver/scanners-test.cc M src/kudu/tserver/scanners.cc M src/kudu/tserver/scanners.h M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tserver.proto M src/kudu/tserver/tserver_path_handlers.cc 11 files changed, 207 insertions(+), 60 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/6348/4 -- To view, visit http://gerrit.cloudera.org:8080/6348 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic91fa0ca471bd674e35aa2f8de3806b88ad4b3b4 Gerrit-Change-Number: 6348 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 ) Change subject: [sentry] add AuthzProvider .. Patch Set 10: Code-Review+2 Looks good to me; maybe Andrew has some more feedback. -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Nov 2018 22:14:03 + Gerrit-HasComments: No
[kudu-CR] revert change to exactly once writes-itest
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11836 ) Change subject: revert change to exactly_once_writes-itest .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11836/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11836/2//COMMIT_MSG@20 PS2, Line 20: which we don't today > Leaks don't cause test failures? When did that change? I wonder if it's whe It seems in some cases, no. I'm still trying to get to the bottom of this, but what I think I'm seeing is that in the EMC process complains about a leak throughout the test, and at the very end when doing the leak check, the leak is somehow now caught. In many, if not most cases, the leak will be caught by the leak check that happens in the shutdown of ExternalDaemon. -- 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: comment 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 Gerrit-Comment-Date: Thu, 01 Nov 2018 22:12:26 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 ) Change subject: [sentry] add AuthzProvider .. Patch Set 10: (3 comments) http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc@57 PS8, Line 57: DEFINE_string(server_name, "server1", > nit: add space Done http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc@90 PS8, Line 90: > nit for this and other timeout-related flags: it seems we have a convention Done http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc@111 PS8, Line 111: > the same unit-related nit: I would expect it to be sentry_service_max_messa Done -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Nov 2018 22:00:33 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11659 to look at the new patch set (#10). Change subject: [sentry] add AuthzProvider .. [sentry] add AuthzProvider This commit adds a high-level abstraction which handles authorizations on Kudu operations, called AuthzProvider. It has a default implementation which always allow any operations for any users, and a SentryAuthzProvider implementation which connects to the Sentry service for authorization metadata. AuthzProvider, along with its implementations, is placed in the master module. The idea is to decouple it from Sentry since in the future, other authorization implementations might be introduced. A follow-up commit will integrate the AuthzProvider into CatalogManager to perform authorization checks on Master RPCs. Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 --- M src/kudu/hms/hms_catalog.cc M src/kudu/master/CMakeLists.txt A src/kudu/master/authz_provider.h A src/kudu/master/default_authz_provider.h A src/kudu/master/sentry_authz_provider-test.cc A src/kudu/master/sentry_authz_provider.cc A src/kudu/master/sentry_authz_provider.h A src/kudu/sentry/sentry-test-base.h M src/kudu/sentry/sentry_action-test.cc M src/kudu/sentry/sentry_action.cc M src/kudu/sentry/sentry_action.h M src/kudu/sentry/sentry_client-test.cc M src/kudu/sentry/sentry_client.cc M src/kudu/sentry/sentry_client.h 14 files changed, 1,003 insertions(+), 116 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/10 -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11659 to look at the new patch set (#9). Change subject: [sentry] add AuthzProvider .. [sentry] add AuthzProvider This commit adds a high-level abstraction which handles authorizations on Kudu operations, called AuthzProvider. It has a default implementation which always allow any operations for any users, and a SentryAuthzProvider implementation which connects to the Sentry service for authorization metadata. AuthzProvider, along with its implementations, is placed in the master module. The idea is to decouple it from Sentry since in the future, other authorization implementations might be introduced. A follow-up commit will integrate the AuthzProvider into CatalogManager to perform authorization checks on Master RPCs. Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 --- M src/kudu/master/CMakeLists.txt A src/kudu/master/authz_provider.h A src/kudu/master/default_authz_provider.h A src/kudu/master/sentry_authz_provider-test.cc A src/kudu/master/sentry_authz_provider.cc A src/kudu/master/sentry_authz_provider.h A src/kudu/sentry/sentry-test-base.h M src/kudu/sentry/sentry_action-test.cc M src/kudu/sentry/sentry_action.cc M src/kudu/sentry/sentry_action.h M src/kudu/sentry/sentry_client-test.cc M src/kudu/sentry/sentry_client.cc M src/kudu/sentry/sentry_client.h 13 files changed, 975 insertions(+), 96 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/9 -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 9 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 ) Change subject: [sentry] add AuthzProvider .. Patch Set 8: Code-Review+1 (3 comments) http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc@57 PS8, Line 57: "Configures which server namespace the Kudu instance belongs to for defining" nit: add space http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc@90 PS8, Line 90: timeout nit for this and other timeout-related flags: it seems we have a convention to reflect time interval units in the name of the flags (e.g., check for various PKI-related parameters in master.cc or consensus-related parameters in raft_consensus.cc/consensus_queue.cc), so I would expect this to be sentry_service_send_timeout_seconds http://gerrit.cloudera.org:8080/#/c/11659/8/src/kudu/master/sentry_authz_provider.cc@111 PS8, Line 111: sentry_service_max_message_size the same unit-related nit: I would expect it to be sentry_service_max_message_size_bytes. -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Nov 2018 20:49:18 + Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11843 ) Change subject: [util] Add ParseStringsWithScheme in net_util .. [util] Add ParseStringsWithScheme in net_util This commit adds a new util to allow parsing address with scheme, e.g. '://:/' into a : pair. Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Reviewed-on: http://gerrit.cloudera.org:8080/11843 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h 3 files changed, 104 insertions(+), 6 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified Alexey Serbin: Looks good to me, but someone else must approve -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [sentry] add AuthzProvider
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11659 to look at the new patch set (#8). Change subject: [sentry] add AuthzProvider .. [sentry] add AuthzProvider This commit adds a high-level abstraction which handles authorizations on Kudu operations, called AuthzProvider. It has a default implementation which always allow any operations for any users, and a SentryAuthzProvider implementation which connects to the Sentry service for authorization metadata. AuthzProvider, along with its implementations, is placed in the master module. The idea is to decouple it from Sentry since in the future, other authorization implementations might be introduced. A follow-up commit will integrate the AuthzProvider into CatalogManager to perform authorization checks on Master RPCs. Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 --- M src/kudu/master/CMakeLists.txt A src/kudu/master/authz_provider.h A src/kudu/master/default_authz_provider.h A src/kudu/master/sentry_authz_provider-test.cc A src/kudu/master/sentry_authz_provider.cc A src/kudu/master/sentry_authz_provider.h A src/kudu/sentry/sentry-test-base.h M src/kudu/sentry/sentry_action-test.cc M src/kudu/sentry/sentry_action.cc M src/kudu/sentry/sentry_action.h M src/kudu/sentry/sentry_client-test.cc M src/kudu/sentry/sentry_client.cc M src/kudu/sentry/sentry_client.h 13 files changed, 974 insertions(+), 96 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11659/8 -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [sentry] add AuthzProvider
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
..
Patch Set 8:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h@33
PS7, Line 33: virtual Status Start() = 0;
:
: // Stops the AuthzProvider instance.
: virtual void Stop() =
> The same reason as for having the AuthorizeXxx() method as pure virtual in
It is hard to say it is not important for Start()/Stop() in that sense, so
updated it.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc@147
PS7, Line 147:
> Oh, I guess 'Authorize' in this sense means 'check the for authorization'.
I will update it to avoid confusion. Thanks!
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc@238
PS7, Line 238:
ASSERT_OK(sentry_authz_provider_->AuthorizeGetTableMetadata("db.table",
kTestUser));
> What happens if Sentry server is not responsive e.g., due network errors or
Yeah, in L249 it is testing what error we are getting when Sentry service is
not responsive. I will add a simulation of busy server too though.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@51
PS7, Line 51: TABLE,
> nit: here and below drop 'virtual' since that's a derived class and the 'ov
Done
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@56
PS7, Line 56: server_name
> Ah, OK. Thank you for clarification.
Done
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@235
PS7, Line 235: RETURN_NOT_OK(Authorize(table_authorizable, table_action,
user));
> I don't think this needs to be a part of the SentryAuthzProvider class. May
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 20:04:05 +
Gerrit-HasComments: Yes
[kudu-CR] [examples] Add basic Spark example (scala)
Mitch Barnett has posted comments on this change. ( http://gerrit.cloudera.org:8080/11788 ) Change subject: [examples] Add basic Spark example (scala) .. Patch Set 11: (13 comments) http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/README.adoc File examples/scala/spark-example/README.adoc: http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/README.adoc@32 PS10, Line 32: To compile and build the example, ensure maven is installed and execute : the > Can you specify what you need for this to work? Do you need Spark running s Done http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/README.adoc@38 PS10, Line 38: $ mvn package > After poking around the internet for a bit, I think we shouldn't try to mak Done http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/README.adoc@43 PS10, Line 43: addr > Remove. Done http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/README.adoc@70 PS10, Line 70: > You can't break the line in the middle of a single-quotes literal, since ev Done http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala File examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala: http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@17 PS10, Line 17: > I think this should be org.apache.kudu.spark.examples. Renamed, and created the additional directory structure. http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@32 PS10, Line 32: > Spaces after commas here and elsewhere. Done http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@40 PS10, Line 40: > Define Done http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@44 PS10, Line 44: > nit: space here Done http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@47 PS10, Line 47: > nit: remove extra line Done http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@51 PS10, Line 51: > KuduSparkExample Done http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@54 PS10, Line 54: > Import Done http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@67 PS10, Line 67: > Let's do the default number of replicas (i.e. remove setting the number of Done http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@102 PS10, Line 102: > . Done -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 11 Gerrit-Owner: Mitch Barnett Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 01 Nov 2018 19:37:12 + Gerrit-HasComments: Yes
[kudu-CR] [examples] Add basic Spark example (scala)
Hello Will Berkeley, Attila Bukor, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11788 to look at the new patch set (#11). Change subject: [examples] Add basic Spark example (scala) .. [examples] Add basic Spark example (scala) This patch adds a basic Kudu-Spark example that utilizes the Kudu-Spark integration. It will allow users to pull down the pom.xml and scala source, then build and execute from their local machine. Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f --- A examples/scala/spark-example/README.adoc A examples/scala/spark-example/pom.xml A examples/scala/spark-example/src/main/scala/org/apache/kudu/spark/examples/SparkExample.scala 3 files changed, 285 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/11788/11 -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 11 Gerrit-Owner: Mitch Barnett Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Will Berkeley
[kudu-CR] [compaction] Cleanup of compaction policy code
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11827 ) Change subject: [compaction] Cleanup of compaction policy code .. Patch Set 5: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.h File src/kudu/tablet/compaction_policy.h: http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.h@66 PS3, Line 66: ompaction policies may prefer to dea > Yes. For two reasons: Ah, I see. Would it be a better choice making this method pure virtual? -- To view, visit http://gerrit.cloudera.org:8080/11827 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I955ec66a510f04fd869f7d969a643e4d0f7f415f Gerrit-Change-Number: 11827 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 01 Nov 2018 18:44:04 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 ) Change subject: [sentry] add AuthzProvider .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc@147 PS7, Line 147: Authorize > Here and below for negative cases: did you actually mean 'Don't authorize' Oh, I guess 'Authorize' in this sense means 'check the for authorization'. If that's a sort of lingo used, that means it was just my misunderstanding: feel free to ignore my comment then. -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Nov 2018 18:29:24 + Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11843 ) Change subject: [util] Add ParseStringsWithScheme in net_util .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 01 Nov 2018 18:24:52 + Gerrit-HasComments: No
[kudu-CR] [sentry] add AuthzProvider
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
..
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h@33
PS7, Line 33: virtual Status Start() { return Status::OK(); }
:
: // Stops the AuthzProvider instance.
: virtual void Stop() {}
> Sure, but any specific reasons for doing this?
The same reason as for having the AuthorizeXxx() method as pure virtual in this
class: make sure particular provider implements those methods. Or
Start()/Stop() are not important in that sense at all?
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 18:15:31 +
Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 ) Change subject: [sentry] add AuthzProvider .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@50 PS7, Line 50: sentry.service.client.server.rpc-addresses > No... this is the name defined in Sentry. Uh-oh... OK. http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@56 PS7, Line 56: server_name > Sentry has server level privilege and this is used to configure that. The i Ah, OK. Thank you for clarification. Could you then add a sentence in the description of this flag to reflect the essence of your explanation above? Something like '... server name of the Kudu cluster used to grant server-level privileges on. Used to distinguish a particular Kudu cluster in case of a multi-cluster setup.' -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Nov 2018 18:12:05 + Gerrit-HasComments: Yes
[kudu-CR] [examples] Add basic Spark example (scala)
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/11788 )
Change subject: [examples] Add basic Spark example (scala)
..
Patch Set 10:
(15 comments)
I tested this locally with a multimaster cluster and with a YARN cluster
running Spark and a single-master Kudu cluster. It worked as expected in both
cases.
However, I think we should remove the ability to run the jar standalone and
require spark-submit to be used. This is, AFAICT, typical for Spark
applications, and it simplifies the configuration of the job.
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/README.adoc
File examples/scala/spark-example/README.adoc:
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/README.adoc@32
PS10, Line 32: To build and run, ensure maven is installed and from the
spark-example directory
: run
Can you specify what you need for this to work? Do you need Spark running
somewhere? A Kudu cluster somewhere?
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/README.adoc@38
PS10, Line 38: $ java -jar target/kudu-spark-example-1.0-SNAPSHOT.jar
After poking around the internet for a bit, I think we shouldn't try to make
this runnable standalone. It should require spark-submit. If we do that, we can
remove the very awkward double specifying of the Spark master.
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/README.adoc@43
PS10, Line 43: Host
Remove.
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/README.adoc@70
PS10, Line 70: \
You can't break the line in the middle of a single-quotes literal, since every
character inside them is treated literally by the shell.
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala
File
examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala:
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@17
PS10, Line 17: org.apache.kudu.examples
I think this should be org.apache.kudu.spark.examples.
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@32
PS10, Line 32: val kuduMasters: String =
System.getProperty("kuduMasters","localhost:7051") // Kudu master address
list.
Spaces after commas here and elsewhere.
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@34
PS10, Line 34: val sparkMaster: String =
System.getProperty("sparkMaster","local")
If we only support running this example with spark-submit, which seems to be
the only thing done in practice, we don't need this parameter, and invoking the
example will be less awkward because the Spark master will only be specified
once, and in the normal Spark way.
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@40
PS10, Line 40: Defining
Define
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@44
PS10, Line 44: case class User(name:String,id:Int)
nit: space here
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@47
PS10, Line 47:
nit: remove extra line
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@51
PS10, Line 51: SparkExample
KuduSparkExample
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@54
PS10, Line 54: Importing
Import
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@67
PS10, Line 67: 1 replica per tablet
Let's do the default number of replicas (i.e. remove setting the number of
replicas explicitly). That lowers the chance for funny business if somebody
copies this and builds it into a production job.
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@102
PS10, Line 102: // Clean up
.
http://gerrit.cloudera.org:8080/#/c/11788/10/examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala@103
PS10, Line 103: $tableName
nit: Here and elsewhere, surround tableName in single quotes.
--
To view, visit http://gerrit.cloudera.org:8080/11788
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id:
[kudu-CR] [sentry] add AuthzProvider
Hao Hao has posted comments on this change. (
http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
..
Patch Set 7:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h@33
PS7, Line 33: virtual Status Start() { return Status::OK(); }
:
: // Stops the AuthzProvider instance.
: virtual void Stop() {}
> nit: if you have DefaultAuthzProvider with dummy implementation of all auth
Sure, but any specific reasons for doing this?
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@270
PS5, Line 270:
> That's what we do for wrapping long lines.
Ah, thanks!
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@55
PS7, Line 55: This following methods will fail if the operation is not
authorized, or the
: // Sentry service is unreachable, or 'kudu' (Kudu server name)
is a non-admin
: // user in Sentry, or Sentry fails to resolve the group mapping
of
: // the user.
> How about we split these out so it's a bit easier to read.
Done
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@90
PS7, Line 90: //
: // This method will fail if the Sentry service is unreachable,
or 'kudu'
: // (Kudu server name) is a non admin user in Sentry, or Sentry
fail to
: // resolve the group mapping of the user.
> nit: not needed given the comment up top?
Done
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@259
PS5, Line 259: return Status::OK();
: }
:
> Ah, thanks for the explanation. So based on where the Authorize() fails, a
Yeah, which we should be careful about what to return in the caller of
AuthzProvider.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@50
PS7, Line 50: sentry.service.client.server.rpc-addresses
> This seems confusing: ...service.client.server Is there any typo in th
No... this is the name defined in Sentry.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@56
PS7, Line 56: server_name
> If that's to describe server namespace in Sentry terms, why not 'sentry_ser
Sentry has server level privilege and this is used to configure that. The idea
is to define server namespace in Kudu to support authorization on multi
clusters. Thus, 'sentry_server_namespace' sounds misleading to me.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@115
PS7, Line 115: on
> super nit: elsewhere we seem to favor capitalizing the whole of "ALL ON DAT
Done
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@209
PS7, Line 209:
> nit: spacing here too
Done
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 17:58:49 +
Gerrit-HasComments: Yes
[kudu-CR](refs/meta/config) Change e-mail notification recipient to reviews@kudu.apache.org
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11840 ) Change subject: Change e-mail notification recipient to reviews@kudu.apache.org .. Patch Set 3: Oops, I pushed to refs/meta/config instead of to refs/for/meta/config, so I wound up merging the change instead of publishing a new revision. Sorry about that. -- To view, visit http://gerrit.cloudera.org:8080/11840 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: refs/meta/config Gerrit-MessageType: comment Gerrit-Change-Id: I45f3a070108eb71931f85c317294cbc90b9bc521 Gerrit-Change-Number: 11840 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 01 Nov 2018 17:43:57 + Gerrit-HasComments: No
[kudu-CR](refs/meta/config) Change e-mail notification recipient to reviews@kudu.apache.org
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11840 ) Change subject: Change e-mail notification recipient to reviews@kudu.apache.org .. Change e-mail notification recipient to reviews@kudu.apache.org We were still using revi...@kudu.incubator.apache.org for some reason. Change-Id: I45f3a070108eb71931f85c317294cbc90b9bc521 --- M project.config 1 file changed, 12 insertions(+), 12 deletions(-) -- To view, visit http://gerrit.cloudera.org:8080/11840 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: refs/meta/config Gerrit-MessageType: merged Gerrit-Change-Id: I45f3a070108eb71931f85c317294cbc90b9bc521 Gerrit-Change-Number: 11840 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Todd Lipcon
[kudu-CR](refs/meta/config) Change e-mail notification recipient to reviews@kudu.apache.org
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11840 ) Change subject: Change e-mail notification recipient to reviews@kudu.apache.org .. Patch Set 3: Verified+1 Code-Review+2 Carrying forward Andrew's +2. -- To view, visit http://gerrit.cloudera.org:8080/11840 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: refs/meta/config Gerrit-MessageType: comment Gerrit-Change-Id: I45f3a070108eb71931f85c317294cbc90b9bc521 Gerrit-Change-Number: 11840 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 01 Nov 2018 17:43:09 + Gerrit-HasComments: No
[kudu-CR](refs/meta/config) Change e-mail notification recipient to reviews@kudu.apache.org
Hello Andrew Wong, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11840 to look at the new patch set (#3). Change subject: Change e-mail notification recipient to reviews@kudu.apache.org .. Change e-mail notification recipient to reviews@kudu.apache.org We were still using revi...@kudu.incubator.apache.org for some reason. Change-Id: I45f3a070108eb71931f85c317294cbc90b9bc521 --- M project.config 1 file changed, 12 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/11840/3 -- To view, visit http://gerrit.cloudera.org:8080/11840 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: refs/meta/config Gerrit-MessageType: newpatchset Gerrit-Change-Id: I45f3a070108eb71931f85c317294cbc90b9bc521 Gerrit-Change-Number: 11840 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [sentry] add AuthzProvider
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider
..
Patch Set 7:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h
File src/kudu/master/authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h@33
PS7, Line 33: virtual Status Start() { return Status::OK(); }
:
: // Stops the AuthzProvider instance.
: virtual void Stop() {}
nit: if you have DefaultAuthzProvider with dummy implementation of all
authz-related methods, why not to make these two methods pure virtual and add
the default implementation for these two methods into DefaultAuthzProvider as
well?
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc
File src/kudu/master/sentry_authz_provider-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc@147
PS7, Line 147: Authorize
Here and below for negative cases: did you actually mean 'Don't authorize' here?
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider-test.cc@238
PS7, Line 238: TEST_P(SentryAuthzProviderTest, TestReconnect) {
What happens if Sentry server is not responsive e.g., due network errors or the
server being too busy and the authz provider tries to get authz info from the
server? Do you think it's worth adding a small test for that case to ensure
the provider behaves appropriately? I think it's pretty easy to simulate a too
busy server: just pause it by sending the SIGSTOP signal to the server process.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h
File src/kudu/master/sentry_authz_provider.h:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@51
PS7, Line 51: virtual
nit: here and below drop 'virtual' since that's a derived class and the
'override' is present for the overridden methods.
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@50
PS7, Line 50: sentry.service.client.server.rpc-addresses
This seems confusing: ...service.client.server Is there any typo in the
name of the property?
http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@56
PS7, Line 56: server_name
If that's to describe server namespace in Sentry terms, why not
'sentry_server_namespace? Is that just for compatibility with Impala's
--server_name flag?
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc@218
PS3, Line 218: set groups;
> It will return an 'Invalid Input' error.
OK, thanks for the info.
--
To view, visit http://gerrit.cloudera.org:8080/11659
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 17:28:23 +
Gerrit-HasComments: Yes
[kudu-CR] [compaction] Cleanup of compaction policy code
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11827 to look at the new patch set (#5). Change subject: [compaction] Cleanup of compaction policy code .. [compaction] Cleanup of compaction policy code I'm reading over the compaction policy code before starting to work in earnest on KUDU-1400, and I made a number of small changes that I think help make the code more readable. This patch contains no functional changes. Change-Id: I955ec66a510f04fd869f7d969a643e4d0f7f415f --- M src/kudu/tablet/compaction_policy-test.cc M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/compaction_policy.h M src/kudu/tablet/svg_dump.cc M src/kudu/tablet/svg_dump.h M src/kudu/tablet/tablet.cc 6 files changed, 292 insertions(+), 266 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/11827/5 -- To view, visit http://gerrit.cloudera.org:8080/11827 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I955ec66a510f04fd869f7d969a643e4d0f7f415f Gerrit-Change-Number: 11827 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11843 ) Change subject: [util] Add ParseStringsWithScheme in net_util .. Patch Set 3: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@153 PS1, Line 153: > Hmm, I did not see how since StripPrefixString or StripSuffixString are rel Ah, yeah I saw what you mean; since the entirety of the scheme and path aren't known up front, you can't use those functions to find them. -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 01 Nov 2018 17:13:20 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 ) Change subject: [sentry] add AuthzProvider .. Patch Set 7: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Nov 2018 17:13:55 + Gerrit-HasComments: No
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11843 ) Change subject: [util] Add ParseStringsWithScheme in net_util .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc File src/kudu/util/net/net_util-test.cc: http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc@110 PS2, Line 110: :1234 > What's wrong with this port? 'abc:1234/path' does not have scheme so valid format would be :, which '1234/path' is considered as the port number. http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc@114 PS2, Line 114: :12 > And this one? Updated. http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc@122 PS2, Line 122: ://scheme > Should this be "invalid scheme" or something? Isn't it the scheme that's in Hmm, makes sense. Updated. -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 01 Nov 2018 17:02:19 + Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11843 to look at the new patch set (#3). Change subject: [util] Add ParseStringsWithScheme in net_util .. [util] Add ParseStringsWithScheme in net_util This commit adds a new util to allow parsing address with scheme, e.g. '://:/' into a : pair. Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 --- M src/kudu/util/net/net_util-test.cc M src/kudu/util/net/net_util.cc M src/kudu/util/net/net_util.h 3 files changed, 104 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/11843/3 -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [compaction] Cleanup of compaction policy code
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11827 to look at the new patch set (#4). Change subject: [compaction] Cleanup of compaction policy code .. [compaction] Cleanup of compaction policy code I'm reading over the compaction policy code before starting to work in earnest on KUDU-1400, and I made a number of small changes that I think help make the code more readable. This patch contains no functional changes. Change-Id: I955ec66a510f04fd869f7d969a643e4d0f7f415f --- M src/kudu/tablet/compaction_policy-test.cc M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/compaction_policy.h M src/kudu/tablet/svg_dump.cc M src/kudu/tablet/svg_dump.h M src/kudu/tablet/tablet.cc 6 files changed, 289 insertions(+), 263 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/27/11827/4 -- To view, visit http://gerrit.cloudera.org:8080/11827 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I955ec66a510f04fd869f7d969a643e4d0f7f415f Gerrit-Change-Number: 11827 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [compaction] Cleanup of compaction policy code
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11827 ) Change subject: [compaction] Cleanup of compaction policy code .. Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy-test.cc File src/kudu/tablet/compaction_policy-test.cc: http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy-test.cc@58 PS3, Line 58: unordered_set > nit: introduce a typedef? Done http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.h File src/kudu/tablet/compaction_policy.h: http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.h@62 PS3, Line 62: size > nit: size in bytes ? Done http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.h@66 PS3, Line 66: std::numeric_limits::max() > Are you sure this is not a functional change? Yes. For two reasons: 1. CompactionPolicy is never instantiated. Only its subclass BudgetedCompactionPolicy is, and it overrides this. 2. 1GB is larger than the default compaction budget of 128MB, so this value would not have been effective anyway. Using uint64_t's max value is more consonant with the "no rolling" intention. http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.h@99 PS3, Line 99: void SetupKnapsackInput(const RowSetTree& tree, > warning: function 'kudu::tablet::BudgetedCompactionPolicy::SetupKnapsackInp Done http://gerrit.cloudera.org:8080/#/c/11827/1/src/kudu/tablet/compaction_policy.h File src/kudu/tablet/compaction_policy.h: http://gerrit.cloudera.org:8080/#/c/11827/1/src/kudu/tablet/compaction_policy.h@a53 PS1, Line 53: > Is this done, or just a non-goal now? Regardless, it'd probably be helpful It's not important (at least right now) what the units are. It's just a double...there's no particular range for it. It tends to be in the range 0-1 for winning selections, but it could be much larger, and it is negative for lots of bad selections. I don't want to refer to the doc here, again. The doc is already referred to a number of times throughout the compaction code. http://gerrit.cloudera.org:8080/#/c/11827/1/src/kudu/tablet/compaction_policy.h@42 PS1, Line 42: ompactionPolicy() = default; : > not needed? The compiler tells me I need it to implicitly initialize the base class in the subclass constructor and also to delete the base class when stored in a smart pointer. http://gerrit.cloudera.org:8080/#/c/11827/1/src/kudu/tablet/compaction_policy.h@100 PS1, Line 100: std::vector* asc_min_key, > warning: function 'kudu::tablet::BudgetedCompactionPolicy::SetupKnapsackInp Done http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.cc File src/kudu/tablet/compaction_policy.cc: http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.cc@53 PS3, Line 53: advanced > As I understand, this change moves the flag into the non-experimental domai Whoops. That was not intentional. Good catch. http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.cc@374 PS3, Line 374: & > style nit: stick the ampersand to the parameter type. Done http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/tablet.cc@2317 PS3, Line 2317: *o > nit: maybe, dereference the pointer just once and use the result reference Done -- To view, visit http://gerrit.cloudera.org:8080/11827 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I955ec66a510f04fd869f7d969a643e4d0f7f415f Gerrit-Change-Number: 11827 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 01 Nov 2018 16:51:40 + Gerrit-HasComments: Yes
[kudu-CR] [util] Add ParseStringsWithScheme in net util
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11843 ) Change subject: [util] Add ParseStringsWithScheme in net_util .. Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc File src/kudu/util/net/net_util-test.cc: http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc@110 PS2, Line 110: :1234 What's wrong with this port? http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc@114 PS2, Line 114: :12 And this one? http://gerrit.cloudera.org:8080/#/c/11843/2/src/kudu/util/net/net_util-test.cc@122 PS2, Line 122: ://scheme Should this be "invalid scheme" or something? Isn't it the scheme that's invalid (since it's empty)? -- To view, visit http://gerrit.cloudera.org:8080/11843 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0 Gerrit-Change-Number: 11843 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 01 Nov 2018 06:25:06 + Gerrit-HasComments: Yes
[kudu-CR] [sentry] add AuthzProvider
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 ) Change subject: [sentry] add AuthzProvider .. Patch Set 7: (7 comments) Mostly just a few nits at this point. http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@270 PS5, Line 270: > I thought wrapped function has 4 space indent? That's what we do for wrapping long lines. >From the GSG >(https://google.github.io/styleguide/cppguide.html#Formatting_Lambda_Expressions): "Format parameters and bodies as for any other function" http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@55 PS7, Line 55: This following methods will fail if the operation is not authorized, or the : // Sentry service is unreachable, or 'kudu' (Kudu server name) is a non-admin : // user in Sentry, or Sentry fails to resolve the group mapping of : // the user. How about we split these out so it's a bit easier to read. The following authorizing methods will fail if: - the operation is not authorized - the Sentry service is unreachable - the specified '--kudu_service_name' is a non-admin user in Sentry - Sentry fails to resolve the group mapping of the user http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@90 PS7, Line 90: // : // This method will fail if the Sentry service is unreachable, or 'kudu' : // (Kudu server name) is a non admin user in Sentry, or Sentry fail to : // resolve the group mapping of the user. nit: not needed given the comment up top? http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@259 PS5, Line 259: return Status::OK(); : } : > For example, if user Bob has ALL privilege on table B and he wants to check Ah, thanks for the explanation. So based on where the Authorize() fails, a malicious user might be able to discover something they shouldn't? http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@115 PS7, Line 115: on super nit: elsewhere we seem to favor capitalizing the whole of "ALL ON DATABASE" http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@209 PS7, Line 209: nit: spacing here too http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@235 PS7, Line 235: Status SentryAuthzProvider::GetAuthorizable(const string& table_name, I don't think this needs to be a part of the SentryAuthzProvider class. Maybe stick it in an anonymous namespace? -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 01 Nov 2018 06:04:03 + Gerrit-HasComments: Yes
