[kudu-CR] KUDU-2099: drop Java 7 support
Adar Dembo has submitted this change and it was merged. (
http://gerrit.cloudera.org:8080/12816 )
Change subject: KUDU-2099: drop Java 7 support
..
KUDU-2099: drop Java 7 support
Two commits merged since 1.9 yield compilation errors when building with
JDK 10 or later:
.../client/AuthzTokenCache.java:230: error: cannot find symbol
List pendingRetries = retriesForTable.putIfAbsent(
^
symbol: method putIfAbsent(String,List)
location: variable retriesForTable of type Map>
.../client/KuduScannerIterator.java:34: error: KuduScannerIterator is not
abstract and does not override abstract method remove() in Iterator
public class KuduScannerIterator implements Iterator {
^
The root cause is that with JDK 10, these two methods have been lifted out
of their respective classes and are now defined via default interface
implementations, but the compiler can't emit those implementations into
Java 7-compatible bytecode[1].
So we can either work around those two issues, or we can drop support for
Java 7. Let's see if there are any objections to the latter.
1. https://stackoverflow.com/a/23757061
Change-Id: I05fcea8bbc7b773f95429b324acd0b9d18a6e9f8
Reviewed-on: http://gerrit.cloudera.org:8080/12816
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong
Reviewed-by: Grant Henke
Reviewed-by: Todd Lipcon
---
M java/README.adoc
M java/gradle.properties
M java/gradle/dependencies.gradle
3 files changed, 3 insertions(+), 8 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, but someone else must approve
Grant Henke: Looks good to me, approved
Todd Lipcon: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/12816
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I05fcea8bbc7b773f95429b324acd0b9d18a6e9f8
Gerrit-Change-Number: 12816
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2750: Add create and alter timestamp to tables
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12842 ) Change subject: KUDU-2750: Add create and alter timestamp to tables .. KUDU-2750: Add create and alter timestamp to tables Add create and last alteration time to every table. The timestamps are persisted with 'SysTablesEntryPB' and displayed on the 'master:port/tables' page. Change-Id: Ife05719cea26b15fdf2b5667b60c3e89947cdda8 Reviewed-on: http://gerrit.cloudera.org:8080/12842 Reviewed-by: Todd Lipcon Tested-by: Kudu Jenkins --- M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto M src/kudu/master/master_path_handlers.cc M www/tables.mustache 4 files changed, 25 insertions(+), 0 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/12842 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ife05719cea26b15fdf2b5667b60c3e89947cdda8 Gerrit-Change-Number: 12842 Gerrit-PatchSet: 3 Gerrit-Owner: helifu Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: helifu
[kudu-CR] [maintenance] Add privilege maintenance thread pool for privilege tables and tablets
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12852 ) Change subject: [maintenance] Add privilege maintenance thread pool for privilege tables and tablets .. Patch Set 1: A few questions about the design/feature: It seems this patch implements a single "privileged" class for tables. Is this flexible enough for all scenarios? I can think of a couple of alternatives: 1) a "boost" which could be applied as a multiplier on top of the op scores for ops in a given tablet. This isn't a guaranteed priority, but can help to make sure that the high-priority tables get prioritized more quickly than others. 2) a more general priority scheme: I think forcing tables into a "normal/high priority" distinction is difficult to manage in a multitenant cluster. I'm afraid this will start getting used, and then at some point someone's going to say "oh, we need a SUPER-HIGH class", etc. Is it worth considering integer priorities? 3) some kind of fair-sharing or budgeting mechanism for maintenance ops across tablets. In other words, in a multitenant use case, we may want to ensure that each tenant gets a fair share of the MM thread pool's resources (perhaps allowing some kind of multiplier/allocation for tenants that are more important). This solves the use case of a single high-priority tenant, but also the more general use case where everyone is equal priority and we don't want a single bad actor to hurt things. That said, another potential issue with MM op prioritization is that flush operations are responsible for keeping memory resources on the tablet server under control. I'm afraid that op prioritization may cause priority inversion: we may choose compactions of high-priority tablets ahead of memory-pressure-reducing flushes of low priority tablets. If the server is under memory pressure, maybe we should ignore priority and flush based on the normal memory-based prioritization? The last question I have is about the use of gflags for this purpose. It seems like this should probably be a table property settable via the API at runtime, rather than a gflag. For example, we could use the 'extra config' functionality provided by https://gerrit.cloudera.org/c/12468/ -- To view, visit http://gerrit.cloudera.org:8080/12852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3ea3b73505157678a8fb551656123b64e6bfb304 Gerrit-Change-Number: 12852 Gerrit-PatchSet: 1 Gerrit-Owner: Yingchun Lai <405403...@qq.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 26 Mar 2019 04:15:40 + Gerrit-HasComments: No
[kudu-CR] KUDU-1711: ColumnSchema supports storing column comment
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/12849 )
Change subject: KUDU-1711: ColumnSchema supports storing column comment
..
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12849/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12849/1//COMMIT_MSG@9
PS1, Line 9: This patch intends to support storing column comment in
ColumnSchema,
One question here (I could probably figure this out but I'm lazy so I'll just
ask):
Do the column comments end up serialized into every WriteRequestPB? I think
currently the client fetches the Schema when it opens a table, and then
includes the schema on every write. We might just copy the whole SchemaPB into
the write, and log it. If users specify lengthy text comments on their columns
this might start to have a noticeable effect on WAL performance, etc.
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema-internal.h
File src/kudu/client/schema-internal.h:
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema-internal.h@117
PS1, Line 117: bool has_comment;
> I see this is the pattern used by the rest of this class, but I wonder why
I think we had some fear of using boost, which might be incorrect
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema.h@245
PS1, Line 245: const std::string& comment() const;
> Since we can't wrap with boost::optional here (client header), we need to f
I think it's reasonable enough to say that "no comment" and empty string are
the same
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/master/catalog_manager.cc@1369
PS1, Line 1369: if (col.comment().length() >
FLAGS_max_column_comment_length) {
we should probably validate that the column is valid UTF8 as well (we can
refactor some code out of ValidateIdentifier above)
--
To view, visit http://gerrit.cloudera.org:8080/12849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82d3228100c262cc4a5c63eaf696927c88bc8990
Gerrit-Change-Number: 12849
Gerrit-PatchSet: 1
Gerrit-Owner: helifu
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 26 Mar 2019 04:23:36 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2750: Add create and alter timestamp to tables
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12842 ) Change subject: KUDU-2750: Add create and alter timestamp to tables .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12842 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ife05719cea26b15fdf2b5667b60c3e89947cdda8 Gerrit-Change-Number: 12842 Gerrit-PatchSet: 2 Gerrit-Owner: helifu Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: helifu Gerrit-Comment-Date: Tue, 26 Mar 2019 04:16:34 + Gerrit-HasComments: No
[kudu-CR](gh-pages) [blog] a blogpost about location awareness in Kudu
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12119 ) Change subject: [blog] a blogpost about location awareness in Kudu .. Patch Set 8: (28 comments) http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md File _posts/2019-03-25-location-awareness.md: http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@1 PS8, Line 1: --- Can you push this to the gh_pages branch in your github fork so a rendered version can be proofed? http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@15 PS8, Line 15: Should this be removed? http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@19 PS8, Line 19: first cut initial implementation? http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@19 PS8, Line 19: starting 1.9.0 ...starting *with the* 1.9.0... http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@20 PS8, Line 20: is built for the following use case: I am not sure this is a "use case" per se, but instead what the term "location awareness" currently means in Kudu. Maybe say something like: "In the initial implementation of location awareness in Kudu, when a Kudu cluster consists of multiple servers spread across several racks, Kudu will place the replicas of a tablet in such a way that the tablet stays available even if all the servers in a single rack become unavailable." http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@26 PS8, Line 26: A rack failure might happen because of a failure of a hardware component shared : among servers in the rack: network switch, power supply, etc. A rack failure can occur when a hardware component shared among servers in the rack, such as a network switch or power supply, fails. http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@31 PS8, Line 31: network latency between datacenters is low. This is a good opportunity to explicitly mention that this is why we call the feature location awareness and not rack awareness. http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@37 PS8, Line 37: are : supposed to should http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@38 PS8, Line 38: physical or cloud-defined hierarchy of the : deployed cluster I am not sure I understand what this means in relation to location awareness utility. I suspect it's saying that the components should map to the hierarchical levels of "failure domains". You could then give a private data center example: `/data-center-0/rack-09` And a cloud example: `/region-0/availability-zone-01` http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@41 PS8, Line 41: However, we want to keep the hierarchy : there to make it possible to exploit it later However, we plan to leverage the hierarchical structure in future releases. http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@43 PS8, Line 43: compatibility with HDFS Perhaps this should be moved up and describe a bit more in detail as a design choice? It's useful to know that you can use the same locations as your HDFS nodes, because it's common to deploy Kudu along size HDFS. http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@52 PS8, Line 52: etc What is the "etc"? What else does it use it for? http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@55 PS8, Line 55: location string for the specified IP address/hostname. The script below specifically shows ip-address. How do I use hostname? http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@59 PS8, Line 59: tablet server restarts Is this dependent on `--follower_unavailable_considered_failed_sec`? Or will a "quick" restart cause the location to be reset? http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@59 PS8, Line 59: Kudu tablet servers are location : agnostic, at least for now, so the assigned location is not reported back : to the tablet server. Maybe this paragraph would flow better if you moved this part to the bottom. That would make it so you describe how the master uses the location configurations, and then tack on at the end that the tablet servers do not need/use it. http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@64 PS8, Line 64: masters provide connected clients How do they do this? http://gerrit.cloudera.org:8080/#/c/12119/8/_posts/2019-03-25-location-awareness.md@62 PS8, Line 62: to try to place replicas evenly across : locations and to keep tablets
[kudu-CR] KUDU-1711: ColumnSchema supports storing column comment
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12849 )
Change subject: KUDU-1711: ColumnSchema supports storing column comment
..
Patch Set 1:
(6 comments)
What happens if you try to add a comment to a column while creating a table in
Impala today? Does table creation fail? Or is the comment added to the HMS
table entry? Asking because I want to make sure there are no compatibility
concerns with respect to Impala.
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema-internal.h
File src/kudu/client/schema-internal.h:
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema-internal.h@117
PS1, Line 117: bool has_comment;
I see this is the pattern used by the rest of this class, but I wonder why they
don't just use boost::optional. Seems a lot easier.
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema.h
File src/kudu/client/schema.h:
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema.h@245
PS1, Line 245: const std::string& comment() const;
Since we can't wrap with boost::optional here (client header), we need to
figure out how to return "no comment". Should we make it the same as the empty
string?
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/client/schema.cc@684
PS1, Line 684: const std::string& KuduColumnSchema::comment() const {
Can drop std:: prefix here.
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/common/schema.h@208
PS1, Line 208:const std::string& comment = "")
If the comment is supposed to be optional, perhaps we can wrap it in
boost::optional to reflect that?
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/common/schema.h@315
PS1, Line 315: COMPARE_COMMENT = 1 << 3,
Do we actually need this broken out into its own comparison mode? Perhaps we
can include it in with COMPARE_DEFAULTS (and change that to something more
generic, like COMPARE_OTHER)?
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:
http://gerrit.cloudera.org:8080/#/c/12849/1/src/kudu/common/wire_protocol.cc@299
PS1, Line 299: std::string comment = "";
Would rather represent "unset" as a non-existent boost::optional.
--
To view, visit http://gerrit.cloudera.org:8080/12849
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82d3228100c262cc4a5c63eaf696927c88bc8990
Gerrit-Change-Number: 12849
Gerrit-PatchSet: 1
Gerrit-Owner: helifu
Gerrit-Reviewer: Adar Dembo
Gerrit-Comment-Date: Tue, 26 Mar 2019 03:50:26 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2750: Add create and alter timestamp to tables
helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12842 ) Change subject: KUDU-2750: Add create and alter timestamp to tables .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master.proto@185 PS1, Line 185: // The create time of the table, in seconds since the epoch. > nit: add "in seconds since the epoch" Done http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master.proto@186 PS1, Line 186: optional int64 create_timestamp = 10; > nit: can you rename this and the below field to '_timestamp' instead of '_t Done http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master.proto@187 PS1, Line 187: // The last alter time of the table, in seconds since the epoch. > same Done http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master_path_handlers.cc@202 PS1, Line 202: str_create_ti > style nit: str_create_time Done http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master_path_handlers.cc@207 PS1, Line 207: table_json["create time"] = EscapeForHtmlToString(str_create_time); > same Done -- To view, visit http://gerrit.cloudera.org:8080/12842 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ife05719cea26b15fdf2b5667b60c3e89947cdda8 Gerrit-Change-Number: 12842 Gerrit-PatchSet: 2 Gerrit-Owner: helifu Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: helifu Gerrit-Comment-Date: Tue, 26 Mar 2019 03:42:52 + Gerrit-HasComments: Yes
[kudu-CR] [maintenance] Add privilege maintenance thread pool for privilege tables and tablets
Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12852 Change subject: [maintenance] Add privilege maintenance thread pool for privilege tables and tablets .. [maintenance] Add privilege maintenance thread pool for privilege tables and tablets This commit add a privilege thread pool in maintenance manager for privilege tables and tablets. In a Kudu cluster with thousands of tables and tablets, it's hard for a specified tablet's maintenance OPs to be launched when their scores are not the highest, even if the table the tablet belongs to is high priority for Kudu users. This patch allow user to specify privilege tables and tablets by gflags, these maintenance OPs of these privilege tables and tablets can be launched in a privilege thread pool, so they can have greater chance to be launched. Change-Id: I3ea3b73505157678a8fb551656123b64e6bfb304 --- M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_metadata.h M src/kudu/tablet/tablet_mm_ops-test.cc M src/kudu/tablet/tablet_mm_ops.cc M src/kudu/tablet/tablet_mm_ops.h M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tablet/tablet_replica_mm_ops.cc M src/kudu/tablet/tablet_replica_mm_ops.h M src/kudu/tserver/tserver_path_handlers.cc M src/kudu/util/maintenance_manager-test.cc M src/kudu/util/maintenance_manager.cc M src/kudu/util/maintenance_manager.h M src/kudu/util/maintenance_manager.proto M www/maintenance-manager.mustache 15 files changed, 445 insertions(+), 154 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/52/12852/1 -- To view, visit http://gerrit.cloudera.org:8080/12852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I3ea3b73505157678a8fb551656123b64e6bfb304 Gerrit-Change-Number: 12852 Gerrit-PatchSet: 1 Gerrit-Owner: Yingchun Lai <405403...@qq.com>
[kudu-CR] KUDU-2750: Add create and alter timestamp to tables
Hello Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12842 to look at the new patch set (#2). Change subject: KUDU-2750: Add create and alter timestamp to tables .. KUDU-2750: Add create and alter timestamp to tables Add create and last alteration time to every table. The timestamps are persisted with 'SysTablesEntryPB' and displayed on the 'master:port/tables' page. Change-Id: Ife05719cea26b15fdf2b5667b60c3e89947cdda8 --- M src/kudu/master/catalog_manager.cc M src/kudu/master/master.proto M src/kudu/master/master_path_handlers.cc M www/tables.mustache 4 files changed, 25 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/12842/2 -- To view, visit http://gerrit.cloudera.org:8080/12842 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ife05719cea26b15fdf2b5667b60c3e89947cdda8 Gerrit-Change-Number: 12842 Gerrit-PatchSet: 2 Gerrit-Owner: helifu Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2744: Expose RPC interface for diff scan
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12592 ) Change subject: KUDU-2744: Expose RPC interface for diff scan .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12592 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9602b549dc499f92b2069b297e66f366e51fb312 Gerrit-Change-Number: 12592 Gerrit-PatchSet: 10 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Tue, 26 Mar 2019 03:38:23 + Gerrit-HasComments: No
[kudu-CR] More Jenkins testing
Mike Percy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12851 Change subject: More Jenkins testing .. More Jenkins testing Plese don't commit this, either Change-Id: I49e9d90cf0631369657c62de6c49d5f4d8f3042f --- M README.adoc 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/12851/1 -- To view, visit http://gerrit.cloudera.org:8080/12851 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I49e9d90cf0631369657c62de6c49d5f4d8f3042f Gerrit-Change-Number: 12851 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Percy
[kudu-CR] KUDU-1711: ColumnSchema supports storing column comment
helifu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12849 Change subject: KUDU-1711: ColumnSchema supports storing column comment .. KUDU-1711: ColumnSchema supports storing column comment This patch intends to support storing column comment in ColumnSchema, and the comment is displayed on the 'master:port/table?id=uuid' and 'tserver:port/tablet?id=uuid'. Note: in this patch c++ client has been added 'Comment()' API, but other clients are not yet. Change-Id: I82d3228100c262cc4a5c63eaf696927c88bc8990 --- M src/kudu/client/client-test.cc M src/kudu/client/schema-internal.h M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/client/table_alterer-internal.cc M src/kudu/common/common.proto M src/kudu/common/schema.cc M src/kudu/common/schema.h M src/kudu/common/wire_protocol.cc M src/kudu/master/catalog_manager.cc M src/kudu/server/webui_util.cc M www/table.mustache M www/tablet.mustache 13 files changed, 195 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/12849/1 -- To view, visit http://gerrit.cloudera.org:8080/12849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I82d3228100c262cc4a5c63eaf696927c88bc8990 Gerrit-Change-Number: 12849 Gerrit-PatchSet: 1 Gerrit-Owner: helifu
[kudu-CR] Jenkins test
Mike Percy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12850 Change subject: Jenkins test .. Jenkins test Do not commit this Change-Id: Ie40be849d1336eae91cc90c3f3e44215aa0c6578 --- M README.adoc 1 file changed, 2 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/50/12850/1 -- To view, visit http://gerrit.cloudera.org:8080/12850 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ie40be849d1336eae91cc90c3f3e44215aa0c6578 Gerrit-Change-Number: 12850 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Percy
[kudu-CR] KUDU-2744: Expose RPC interface for diff scan
Hello Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12592 to look at the new patch set (#10). Change subject: KUDU-2744: Expose RPC interface for diff scan .. KUDU-2744: Expose RPC interface for diff scan This patch exposes an RPC interface for diff scans by adding a new field "snap_start_timestamp" to NewScanRequestPB. Additional validation is performed to ensure this timestamp is not prior to the ancient history mark. Includes a randomized test of the raw RPC interface. Change-Id: I9602b549dc499f92b2069b297e66f366e51fb312 --- M src/kudu/integration-tests/tablet_history_gc-itest.cc M src/kudu/tools/ksck_remote-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h M src/kudu/tserver/tserver.proto 6 files changed, 308 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/12592/10 -- To view, visit http://gerrit.cloudera.org:8080/12592 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9602b549dc499f92b2069b297e66f366e51fb312 Gerrit-Change-Number: 12592 Gerrit-PatchSet: 10 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR](gh-pages) [blog] a blogpost about location awareness in Kudu
Hello Will Berkeley, Greg Solovyev, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12119 to look at the new patch set (#8). Change subject: [blog] a blogpost about location awareness in Kudu .. [blog] a blogpost about location awareness in Kudu Change-Id: I10b30a80d5661fb889a11285b8118cdea1a87cd2 --- A _posts/2019-03-25-location-awareness.md 1 file changed, 348 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/12119/8 -- To view, visit http://gerrit.cloudera.org:8080/12119 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: gh-pages Gerrit-MessageType: newpatchset Gerrit-Change-Id: I10b30a80d5661fb889a11285b8118cdea1a87cd2 Gerrit-Change-Number: 12119 Gerrit-PatchSet: 8 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] tablet service: improve error handling specificity
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12840 ) Change subject: tablet_service: improve error handling specificity .. Patch Set 3: Code-Review+2 The test failures look legit. -- To view, visit http://gerrit.cloudera.org:8080/12840 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e6f7b01336e78ea8f37cb17b2c4485138e23581 Gerrit-Change-Number: 12840 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Mon, 25 Mar 2019 23:15:37 + Gerrit-HasComments: No
[kudu-CR] WIP [master] introduced SentryAuthzCache
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12833 )
Change subject: WIP [master] introduced SentryAuthzCache
..
Patch Set 4:
(6 comments)
Haven't reviewed yet, just flipping through.
http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc
File src/kudu/master/sentry_authz_provider.cc:
http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc@111
PS4, Line 111: DEFINE_uint32(sentry_authz_cache_capacity_bytes, 256 * 1024 *
1024,
How did you arrive at this default value?
http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc@116
PS4, Line 116: TAG_FLAG(sentry_authz_cache_capacity_bytes, experimental);
Of the new flags, this one doesn't necessarily seem experimental.
http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc@331
PS4, Line 331: vector AuthzInfoKey::GenerateKeySequence(
Would be more efficient to break this out into the N possible cases and do a
single strings::Substitute or equivalent for each. gutils' StrCat or StrAppend
may also be useful here.
http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc@369
PS4, Line 369: size_t res = sizeof(TListSentryPrivilegesResponse);
Instead of this, pass in the pointer itself and call kudu_malloc_usable_size()
on it.
http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc@530
PS4, Line 530: unique_ptr res_ptr;
Could you allocate an empty TListSentryPrivilegesResponse here, and use it for
SendRequestToSentry and memory_footprint?
http://gerrit.cloudera.org:8080/#/c/12833/4/src/kudu/master/sentry_authz_provider.cc@582
PS4, Line 582: if (cache_capacity_bytes > 0) {
Seems like you want to call cache_.reset() either way if the goal is to
accommodate a change to FLAGS_sentry_authz_cache_capacity_bytes, no?
If not, ignore this.
--
To view, visit http://gerrit.cloudera.org:8080/12833
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idaefacd50736f1f152dae34e76778e17b2e84cbe
Gerrit-Change-Number: 12833
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 25 Mar 2019 23:14:32 +
Gerrit-HasComments: Yes
[kudu-CR] WIP [util] introduce TTL cache
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12825 )
Change subject: WIP [util] introduce TTL cache
..
Patch Set 3:
(27 comments)
http://gerrit.cloudera.org:8080/#/c/12825/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12825/3//COMMIT_MSG@12
PS3, Line 12: Expired entries are not returned from the cache, but kept around
until
: they are lazily purged upon cache reaching its capacity while
: accommodating more entries.
Curious if you want to introduce a singleton thread that wakes up periodically
to purge expired entries. Otherwise the cache capacity is never reclaimed, even
if the only workload to use it is long gone.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@55
PS3, Line 55: using TTLTestCache = TTLCache;
Seems like you could hardcode string as the key too, given how you're testing.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@62
PS3, Line 62: FLAGS_cache_force_single_shard = true;
: FLAGS_cache_memtracker_approximation_ratio = 0;
Doc why you set these.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@68
PS3, Line 68: auto h = cache.Get("key0");
: ASSERT_EQ(nullptr, h.get());
Could combine negative lookups onto one line.
Below too.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@118
PS3, Line 118: ASSERT_NE(nullptr, put_h.get());
: ASSERT_NE(nullptr, put_h->ptr());
: ASSERT_EQ(100, put_h->ptr()->value);
This seems to be a common pattern; maybe encapsulate in a helper method?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@155
PS3, Line 155: auto h = cache.Get(key);
: ASSERT_NE(nullptr, h.get());
: ASSERT_NE(nullptr, h->ptr());
: ASSERT_EQ(i, h->ptr()->value);
Any concern here (or on L170) that the test will run sufficiently slow that the
entries may be evicted by the time we call Get()? Could you loop in TSAN mode
with stress threads?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@201
PS3, Line 201: ASSERT_NE(nullptr, h.get());
Unnecessarily duplicated
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@40
PS3, Line 40: // A TTL cache. It's a cache of limited capacity with FIFO
eviction policy.
: // All cache's entries have the same TTL. The TTL for cache's
entries is
: // specified at the creation time of the cache and does not
change during its
: // lifetime.
Maybe reword as: "Cache with TTL-based time eviction and FIFO-based capacity
eviction. All entries have the same static TTL specified at cache creation
time."
Should also mention here (or maybe in Get()/Put()) how returned handles affect
eviction (either time or capacity based) as well as destruction of the
underlying cached value. I think the answers are "cached key/value pairs may
still be evicted even with an outstanding handle, but a cached value won't be
destroyed until a returned value goes out of scope".
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@59
PS3, Line 59: V* ptr() const {
: return ptr_;
: }
Why offer this at all? Is mutating a cached key's value an important use case?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@91
PS3, Line 91: // TODO(aserbin): maybe, prohibit inheriting from this class
instead?
I don't think we need to be super protective on these matters; I think it's
safe to assume that nobody is going to extend this class, so a virtual
destructor is unnecessary.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@93
PS3, Line 93:
: // Retrieve an entry from the cache and return it wrapped into
a ref-counting
: // handle. If a non-expired entry exists for the specified key,
this method
: // returns shared pointer for non-null handle. If there is no
entry for the
: // specified key or the entry has expired at the time of the
query,
: // nullptr wrapped into a ref-counted handle is returned.
This (and Put()) is a little too much detail. The general "retrieves...if an
entry exists...if no entry exists" structure is good, but I think you can elide
some of the associated detail, especially since the implementations are inline
and can be inspected easily.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@99
PS3, Line 99: std::shared_ptr Get(const K& key) {
Why do
[kudu-CR] [KUDU-1344] Part 1: cmake install for executables
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12847 ) Change subject: [KUDU-1344] Part 1: cmake install for executables .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12847/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12847/1//COMMIT_MSG@18 PS1, Line 18: This change also adds 3 cmake flags (off by default): > instead of the "double negative" flags, can we add on-by-default flags whic Good point about double negative flags. -- To view, visit http://gerrit.cloudera.org:8080/12847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic020941343efc9192bf615c0c8a30b3d9eee91d7 Gerrit-Change-Number: 12847 Gerrit-PatchSet: 1 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 25 Mar 2019 22:46:13 + Gerrit-HasComments: Yes
[kudu-CR] [KUDU-1344] Part 1: cmake install for executables
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12847 ) Change subject: [KUDU-1344] Part 1: cmake install for executables .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/12847/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12847/1//COMMIT_MSG@18 PS1, Line 18: This change also adds 3 cmake flags (off by default): instead of the "double negative" flags, can we add on-by-default flags which enable the install? eg by adding in the top-level CMakeLists.txt something like: set(KUDU_CLIENT_INSTALL ON CACHE BOOL "Whether to install the Kudu client library") (see https://cmake.org/cmake/help/v2.8.8/cmake.html#command%3aset) -- To view, visit http://gerrit.cloudera.org:8080/12847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic020941343efc9192bf615c0c8a30b3d9eee91d7 Gerrit-Change-Number: 12847 Gerrit-PatchSet: 1 Gerrit-Owner: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 25 Mar 2019 22:34:10 + Gerrit-HasComments: Yes
[kudu-CR] [KUDU-1344] Part 1: cmake install for executables
Greg Solovyev has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12847 Change subject: [KUDU-1344] Part 1: cmake install for executables .. [KUDU-1344] Part 1: cmake install for executables This change adds cmake install commands for kudu, kudu-tserver and kudu-master executables. Running 'sudo make install' installs the following: - kudu-tserver and kudu-master executables in /usr/local/sbin - Kudu client executable in /usr/local/bin - Kudu client library in /usr/local/lib/ - Kudu client headers in /usr/local/include/kudu This change also adds 3 cmake flags (off by default): - KUDU_SKIP_CLIENT_INSTALL - KUDU_SKIP_TSERVER_INSTALL - KUDU_SKIP_MASTER_INSTALL setting these flags to ON directs cmake to skip corresponding install commands. Update documentation to reflect these changes. Change-Id: Ic020941343efc9192bf615c0c8a30b3d9eee91d7 --- M docs/installation.adoc M src/kudu/master/CMakeLists.txt M src/kudu/tools/CMakeLists.txt M src/kudu/tserver/CMakeLists.txt 4 files changed, 123 insertions(+), 47 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/12847/1 -- To view, visit http://gerrit.cloudera.org:8080/12847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ic020941343efc9192bf615c0c8a30b3d9eee91d7 Gerrit-Change-Number: 12847 Gerrit-PatchSet: 1 Gerrit-Owner: Greg Solovyev
[kudu-CR] tablet service: improve error handling specificity
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12840 to look at the new patch set (#3). Change subject: tablet_service: improve error handling specificity .. tablet_service: improve error handling specificity The error handling in TabletServiceImpl::HandleNewScanRequest() was convoluted. It's still not particularly inspired, but this patch makes several clarity and maintainability improvements: * Instead of making it the default error returned from HandleNewScanRequest(), localize the return of TabletServerErrorPB::MISMATCHED_SCHEMA to where Iterator::Init() returns an InvalidArgument, since that comes from a GetMappedReadProjection() failure. * Use an INVALID_SCAN_SPEC error for the case where the user specifies an unrecognized order mode, which is consistent with when the user specifies an unknown read mode or when the user specifies order mode ORDERED without specifying a snapshot scan. Also, update the comments in the .proto file to reflect the current usage. * Localize assignment of TabletServerErrorPB::Code as much as possible in HandleNewScanRequest() by plumbing an `error_code` pointer into HandleScanAtSnapshot() so that errors in that method can be specific to the type of problem observed. Change-Id: I1e6f7b01336e78ea8f37cb17b2c4485138e23581 --- M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h M src/kudu/tserver/tserver.proto 6 files changed, 79 insertions(+), 57 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/12840/3 -- To view, visit http://gerrit.cloudera.org:8080/12840 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1e6f7b01336e78ea8f37cb17b2c4485138e23581 Gerrit-Change-Number: 12840 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] tablet service: improve error handling specificity
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12840 ) Change subject: tablet_service: improve error handling specificity .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_service.cc@2402 PS1, Line 2402: Status s = PickAndVerifyTimestamp(scan_pb, tablet, _snap_timestamp); > Right, that's why I added _PREPEND_ in there (though there's no variant for Oh, I missed that part. The name starts to get so dang long. I also am not really convinced this is a prevalent-enough pattern to warrant a helper macro at this time, but would not be against adding it if we thought it would be used a lot. http://gerrit.cloudera.org:8080/#/c/12840/2/src/kudu/util/status.h File src/kudu/util/status.h: http://gerrit.cloudera.org:8080/#/c/12840/2/src/kudu/util/status.h@128 PS2, Line 128: #define RETURN_NOT_OK_CALLKUDU_RETURN_NOT_OK_CALL > Need to remove this. thanks -- To view, visit http://gerrit.cloudera.org:8080/12840 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e6f7b01336e78ea8f37cb17b2c4485138e23581 Gerrit-Change-Number: 12840 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Mon, 25 Mar 2019 22:25:55 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2744: Expose RPC interface for diff scan
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12592 ) Change subject: KUDU-2744: Expose RPC interface for diff scan .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12592 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9602b549dc499f92b2069b297e66f366e51fb312 Gerrit-Change-Number: 12592 Gerrit-PatchSet: 9 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Mon, 25 Mar 2019 21:27:51 + Gerrit-HasComments: No
[kudu-CR] tablet service: improve error handling specificity
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12840 ) Change subject: tablet_service: improve error handling specificity .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_service.cc@2402 PS1, Line 2402: Status s = PickAndVerifyTimestamp(scan_pb, tablet, _snap_timestamp); > not really, because of the CloneAndPrepend() Right, that's why I added _PREPEND_ in there (though there's no variant for this right now). Anyway, probably not worth adding a variant for just this case, unless it's more prevalent across the code base. http://gerrit.cloudera.org:8080/#/c/12840/2/src/kudu/util/status.h File src/kudu/util/status.h: http://gerrit.cloudera.org:8080/#/c/12840/2/src/kudu/util/status.h@128 PS2, Line 128: #define RETURN_NOT_OK_CALLKUDU_RETURN_NOT_OK_CALL Need to remove this. -- To view, visit http://gerrit.cloudera.org:8080/12840 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e6f7b01336e78ea8f37cb17b2c4485138e23581 Gerrit-Change-Number: 12840 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Mon, 25 Mar 2019 21:27:26 + Gerrit-HasComments: Yes
[kudu-CR] tablet server-test-base: make types match schema
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12839 ) Change subject: tablet_server-test-base: make types match schema .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12839 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I209db29d8be884db1173f5141f2e63a89c32d408 Gerrit-Change-Number: 12839 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Mon, 25 Mar 2019 21:23:47 + Gerrit-HasComments: No
[kudu-CR] WIP [util] introduce TTL cache
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12825
to look at the new patch set (#3).
Change subject: WIP [util] introduce TTL cache
..
WIP [util] introduce TTL cache
This changelist introduces TTL cache based on the cache implementation
from util/cache.{h,cc} with FIFO eviction policy.
Expired entries are not returned from the cache, but kept around until
they are lazily purged upon cache reaching its capacity while
accommodating more entries.
WIP:
* introduce and update TTL-related metric gauges
Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
A src/kudu/util/ttl_cache-test.cc
A src/kudu/util/ttl_cache.h
A src/kudu/util/ttl_cache_metrics.cc
A src/kudu/util/ttl_cache_metrics.h
6 files changed, 508 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12825/3
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] WIP [master] introduced SentryAuthzCache
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12833 to look at the new patch set (#4). Change subject: WIP [master] introduced SentryAuthzCache .. WIP [master] introduced SentryAuthzCache This patch incorporates a TTL-based cache into the SentryAuthzProvider. The cache stores responses received from the authz provider, i.e. Sentry in this particular case. It's possible to enable or disable using the caching of Sentry responses upon creation of SentryAuthzProvider instance: set the newly introduced `--sentry_authz_cache_capacity_bytes` command-line flag to 0 to disable the cache. In addition, it's possible to force the cache to fetch and cache information from higher level of Sentry's authz hierarchy: use the `--sentry_authz_cache_finest_scope` flag for that. WIP: * to add tests specific to SentryAuthzCache: ** for various --sentry_authz_cache_finest_scope Change-Id: Idaefacd50736f1f152dae34e76778e17b2e84cbe --- M src/kudu/master/CMakeLists.txt M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_authz_provider.cc M src/kudu/master/sentry_authz_provider.h 4 files changed, 394 insertions(+), 73 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/12833/4 -- To view, visit http://gerrit.cloudera.org:8080/12833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idaefacd50736f1f152dae34e76778e17b2e84cbe Gerrit-Change-Number: 12833 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] WIP [util] introduce TTL cache
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/12825 )
Change subject: WIP [util] introduce TTL cache
..
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache-test.cc@35
PS2, Line 35: using std::shared_ptr;
> warning: using decl 'shared_ptr' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@41
PS2, Line 41: entiries
> nit: entries
Done
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@42
PS2, Line 42: do
> nit: does
Done
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@63
PS2, Line 63: V& value() const {
: DCHECK(ptr_);
: return *ptr_;
: }
> Is this used anywhere right now?
Yes, after a few iterations on authz provider that's now used in
sentry_authz_provider.
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@109
PS2, Line 109: // If the item has expired already, return null handle. The
underlying
: // cache will purge the expired entry when necessary upon
accommodating
: // new entries being added into the cache, if any.
> Curious what the line of thought behind this is -- it seems innocuous enoug
The way how the TTL cache is used assumes the client will fetch and put a new
entry once it's detected an entry is expired. At least for the current use
case in Sentry authz provider that's the case.
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@128
PS2, Line 128: size_t charge
> Perhaps it's worth having a default of kAutomaticCharge?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 25 Mar 2019 19:45:12 +
Gerrit-HasComments: Yes
[kudu-CR] add document for KUDU-2080
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12774 ) Change subject: add document for KUDU-2080 .. Patch Set 1: Code-Review+1 +1 (please address Adar's comment on wording) -- To view, visit http://gerrit.cloudera.org:8080/12774 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7a802a846ad5ec93ce4e0022ec279f1b4c6cc5db Gerrit-Change-Number: 12774 Gerrit-PatchSet: 1 Gerrit-Owner: Jia Hongchao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 25 Mar 2019 18:40:55 + Gerrit-HasComments: No
[kudu-CR] [java] Add private diff scan support
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12689 ) Change subject: [java] Add private diff scan support .. Patch Set 15: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12689 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I51b01ae76cd22407df9097b56629ac7262ec2964 Gerrit-Change-Number: 12689 Gerrit-PatchSet: 15 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Mon, 25 Mar 2019 18:00:10 + Gerrit-HasComments: No
[kudu-CR] [sentry] Integrate AuthzProvider into CatalogManager
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11797 ) Change subject: [sentry] Integrate AuthzProvider into CatalogManager .. [sentry] Integrate AuthzProvider into CatalogManager This commit enables master RPC authorization enforcement by connecting the CatalogManager to the Sentry service via the SentryAuthzProvider. When the Sentry integration is enabled (by setting the --sentry_service_rpc_addresses flag), DDLs such as table creation, alteration, deletion are validated to see if the connected user has the permission to perform such operations. Note that the coarse-grained access control is still applied to these endpoints. A --trusted_user_acl flag is introduced to allow the trusted user, e.g. 'impala', to skip the authorization enforcement. Testing: This commit adds a new integration test (master_sentry-itest) which tests that the integration works as expected with all exposed table operations. More coverage on DDL stress tests with Sentry integration enabled will be in a follow up patch. Change-Id: Iab4aa027ae6eb4520db48ce348db552c9feec2a8 Reviewed-on: http://gerrit.cloudera.org:8080/11797 Tested-by: Hao Hao Reviewed-by: Andrew Wong --- M src/kudu/client/client-test.cc M src/kudu/common/table_util-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/consistency-itest.cc M src/kudu/integration-tests/create-table-stress-test.cc M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/flex_partitioning-itest.cc A src/kudu/integration-tests/hms_itest-base.cc A src/kudu/integration-tests/hms_itest-base.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/integration-tests/master_sentry-itest.cc M src/kudu/integration-tests/raft_config_change-itest.cc M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc M src/kudu/integration-tests/registration-test.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/tombstoned_voting-itest.cc M src/kudu/master/CMakeLists.txt A src/kudu/master/authz_provider.cc M src/kudu/master/authz_provider.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master-test-util.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc A src/kudu/master/sentry_authz_provider-test-base.h M src/kudu/master/sentry_authz_provider-test.cc M src/kudu/master/sentry_authz_provider.cc M src/kudu/master/sentry_authz_provider.h M src/kudu/sentry/mini_sentry.cc M src/kudu/tools/rebalancer_tool-test.cc 33 files changed, 1,549 insertions(+), 481 deletions(-) Approvals: Hao Hao: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Iab4aa027ae6eb4520db48ce348db552c9feec2a8 Gerrit-Change-Number: 11797 Gerrit-PatchSet: 17 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] KUDU-2744: Expose RPC interface for diff scan
Hello Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12592 to look at the new patch set (#9). Change subject: KUDU-2744: Expose RPC interface for diff scan .. KUDU-2744: Expose RPC interface for diff scan This patch exposes an RPC interface for diff scans by adding a new field "snap_start_timestamp" to NewScanRequestPB. Additional validation is performed to ensure this timestamp is not prior to the ancient history mark. Includes a randomized test of the raw RPC interface. Change-Id: I9602b549dc499f92b2069b297e66f366e51fb312 --- M src/kudu/integration-tests/tablet_history_gc-itest.cc M src/kudu/tools/ksck_remote-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h M src/kudu/tserver/tserver.proto 6 files changed, 309 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/12592/9 -- To view, visit http://gerrit.cloudera.org:8080/12592 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9602b549dc499f92b2069b297e66f366e51fb312 Gerrit-Change-Number: 12592 Gerrit-PatchSet: 9 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] tablet service: improve error handling specificity
Mike Percy has posted comments on this change. (
http://gerrit.cloudera.org:8080/12840 )
Change subject: tablet_service: improve error handling specificity
..
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12840/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12840/1//COMMIT_MSG@20
PS1, Line 20: order mode
> ORDERED here, right?
Done
http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_server-test-base.h
File src/kudu/tserver/tablet_server-test-base.h:
http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_server-test-base.h@116
PS1, Line 116: void VerifyScanRequestFailure(const ScanRequestPB& req,
> Maybe pass 'req' by value and std::move() from the two call-sites?
There's no perf benefit to that because the RPC proxy currently takes a const
ref.
http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_server-test.cc@2433
PS1, Line 2433: // Protobuf will parse an unrecognized enum value as
UNKNOWN_ORDER_MODE.
> Nit: this is meant to rationalize why L2434 is passing UNKNOWN_ORDER_MODE i
Done
http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_service.cc@2160
PS1, Line 2160: [&] { *error_code =
TabletServerErrorPB::INVALID_SNAPSHOT; });
> Couldn't you do this via the existing RETURN_NOT_OK_EVAL also?
That's a good point. I wasn't sure it would work, but it turns out that a macro
can treat an assignment as an argument so I'll just switch to that.
http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_service.cc@2402
PS1, Line 2402: Status s = PickAndVerifyTimestamp(scan_pb, tablet,
_snap_timestamp);
> Seems like a good place to use RETURN_NOT_OK_PREPEND_{CALL,EVAL} :)
not really, because of the CloneAndPrepend()
http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_service.cc@2428
PS1, Line 2428:
: tablet::MvccSnapshot snap;
: tablet::MvccManager* mvcc_manager = tablet->mvcc_manager();
> Can these be scoped within block on L2431?
No, but we can get rid of the shorthand mvcc_manager so I'll go ahead and do
that.
--
To view, visit http://gerrit.cloudera.org:8080/12840
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6f7b01336e78ea8f37cb17b2c4485138e23581
Gerrit-Change-Number: 12840
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Comment-Date: Mon, 25 Mar 2019 17:54:25 +
Gerrit-HasComments: Yes
[kudu-CR] [sentry] Integrate AuthzProvider into CatalogManager
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 ) Change subject: [sentry] Integrate AuthzProvider into CatalogManager .. Patch Set 16: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4aa027ae6eb4520db48ce348db552c9feec2a8 Gerrit-Change-Number: 11797 Gerrit-PatchSet: 16 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: Mon, 25 Mar 2019 18:03:30 + Gerrit-HasComments: No
[kudu-CR] [sentry] Integrate AuthzProvider into CatalogManager
Hao Hao has removed a vote on this change. Change subject: [sentry] Integrate AuthzProvider into CatalogManager .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Iab4aa027ae6eb4520db48ce348db552c9feec2a8 Gerrit-Change-Number: 11797 Gerrit-PatchSet: 16 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] Integrate AuthzProvider into CatalogManager
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 ) Change subject: [sentry] Integrate AuthzProvider into CatalogManager .. Patch Set 16: Verified+1 Unrelated flaky test(HmsConfigurations/MasterFailoverTest) -- To view, visit http://gerrit.cloudera.org:8080/11797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4aa027ae6eb4520db48ce348db552c9feec2a8 Gerrit-Change-Number: 11797 Gerrit-PatchSet: 16 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: Mon, 25 Mar 2019 17:57:10 + Gerrit-HasComments: No
[kudu-CR] tablet service: improve error handling specificity
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12840 to look at the new patch set (#2). Change subject: tablet_service: improve error handling specificity .. tablet_service: improve error handling specificity The error handling in TabletServiceImpl::HandleNewScanRequest() was convoluted. It's still not particularly inspired, but this patch makes several clarity and maintainability improvements: * Instead of making it the default error returned from HandleNewScanRequest(), localize the return of TabletServerErrorPB::MISMATCHED_SCHEMA to where Iterator::Init() returns an InvalidArgument, since that comes from a GetMappedReadProjection() failure. * Use an INVALID_SCAN_SPEC error for the case where the user specifies an unrecognized order mode, which is consistent with when the user specifies an unknown read mode or when the user specifies order mode ORDERED without specifying a snapshot scan. Also, update the comments in the .proto file to reflect the current usage. * Localize assignment of TabletServerErrorPB::Code as much as possible in HandleNewScanRequest() by plumbing an `error_code` pointer into HandleScanAtSnapshot() so that errors in that method can be specific to the type of problem observed. Change-Id: I1e6f7b01336e78ea8f37cb17b2c4485138e23581 --- M src/kudu/tserver/tablet_server-test-base.cc M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_service.cc M src/kudu/tserver/tablet_service.h M src/kudu/tserver/tserver.proto M src/kudu/util/status.h 7 files changed, 80 insertions(+), 57 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/12840/2 -- To view, visit http://gerrit.cloudera.org:8080/12840 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1e6f7b01336e78ea8f37cb17b2c4485138e23581 Gerrit-Change-Number: 12840 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] tablet server-test-base: make types match schema
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12839 to look at the new patch set (#2). Change subject: tablet_server-test-base: make types match schema .. tablet_server-test-base: make types match schema This patch makes the types used in the TabletServerTestBase helper methods consistent with the schema they assume, which specifies an int32 for both the key and the value fields. Additionally, fix up the variable types in various places in tablet_server-test to match the calls that are assigned to them. Change-Id: I209db29d8be884db1173f5141f2e63a89c32d408 --- 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 3 files changed, 30 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/12839/2 -- To view, visit http://gerrit.cloudera.org:8080/12839 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I209db29d8be884db1173f5141f2e63a89c32d408 Gerrit-Change-Number: 12839 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] tablet server-test-base: make types match schema
Mike Percy has posted comments on this change. (
http://gerrit.cloudera.org:8080/12839 )
Change subject: tablet_server-test-base: make types match schema
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12839/1/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/12839/1/src/kudu/tserver/tablet_server-test.cc@2217
PS1, Line 2217: for (int32_t i = 1; i < 100; i++) {
> Nit: this could probably just a regular int as it's a totally unspecial loo
Fair point, fixed
http://gerrit.cloudera.org:8080/#/c/12839/1/src/kudu/tserver/tablet_server-test.cc@2991
PS1, Line 2991: int32_t warmup = AllowSlowTests() ?
> Likewise, seems like a regular int is fine here and for max_rows.
Done
--
To view, visit http://gerrit.cloudera.org:8080/12839
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I209db29d8be884db1173f5141f2e63a89c32d408
Gerrit-Change-Number: 12839
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Comment-Date: Mon, 25 Mar 2019 17:54:09 +
Gerrit-HasComments: Yes
[kudu-CR] authz: verify tokens on scans
Andrew Wong has submitted this change and it was merged. (
http://gerrit.cloudera.org:8080/11753 )
Change subject: authz: verify tokens on scans
..
authz: verify tokens on scans
Adds privilege checking to enforce the following authorization
requirements are met when scan-like requests are received by tablet
servers:
Scans or checksum scans require:
if no projected columns || projected columns has virtual column:
foreach (column): SCAN ON COLUMN
else:
if uses pk:
foreach(primary key column): SCAN ON COLUMN
foreach(projected column): SCAN ON COLUMN
foreach(predicated column): SCAN ON COLUMN
Split-key requests require:
if uses pk:
foreach(primary key column): SCAN ON COLUMN
foreach(requested column): SCAN ON COLUMN
Notes:
Empty projections
- Kudu uses this to implement counting rows, which is semantically
equivalent to counting rows with a projection on all columns.
Primary keys
- Scans in ORDERED mode (i.e. fault-tolerant scans) pass around
primary keys to keep track of scan progress.
- Scans that include a start or stop primary key will use the bounds
as a range predicate on the primary key columns. Split-key requests
use similar fields.
Virtual columns
- Diff scans are implemented by having users supply a column in the
projection that doesn't exist in the tablet schema. As an example,
in a table where the column "deleted" does not exist, a diff scan
looks like:
projection: ("col0", type:string), ("deleted", type:is_deleted)
whereas a projection with a column that doesn't exist might look
like the following (note the only difference is in the type):
projection: ("col0", type:string), ("deleted", type:string)
- In the latter case, in order to prevent leaking the existence (or
lack there of, in this case) of the column "deleted", we send back
an authorization error instead of a "not found" error.
- In the former case, we actually want the request to proceed. Even
though the column doesn't exist, we don't expect it to because it
has a virtual type. Therein lies room for a vulnerability -- if a
malicious user were to replace the types in the request with a
virtual column type, there wouldn't be a good way to distinguish
between these two cases.
- To reconcile this, we apply the most conservative policy we can and
require that virtual columns require privileges on all columns.
All of the listed requests are also permitted if SCAN ON TABLE (i.e.
full scan privileges) are given.
Change-Id: I7a5d81cf215a5d936f8853feba05778038764905
Reviewed-on: http://gerrit.cloudera.org:8080/11753
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo
Reviewed-by: Andrew Wong
---
M src/kudu/common/schema.h
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/tablet_server_authorization-test.cc
M src/kudu/tserver/tablet_service.cc
4 files changed, 1,047 insertions(+), 57 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, but someone else must approve
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/11753
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7a5d81cf215a5d936f8853feba05778038764905
Gerrit-Change-Number: 11753
Gerrit-PatchSet: 16
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] authz: verify tokens on scans
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11753 ) Change subject: authz: verify tokens on scans .. Patch Set 15: Code-Review+2 Rolling forward Hao's +2 since all that has changed is the commit message. -- To view, visit http://gerrit.cloudera.org:8080/11753 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7a5d81cf215a5d936f8853feba05778038764905 Gerrit-Change-Number: 11753 Gerrit-PatchSet: 15 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo 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: Mon, 25 Mar 2019 17:19:17 + Gerrit-HasComments: No
[kudu-CR] tserver: sanitize write op types
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12815 ) Change subject: tserver: sanitize write op types .. tserver: sanitize write op types We previously wouldn't make sure that Write requests actually contained write operations (i.e. INSERT, UPDATE, UPSERT, DELETE). The result is that a malicious user could send a bad write request to crash a tablet server. This patch addresses this by adding different decoder modes (e.g. for writes, for split rows), and using the appropriate decoding mode for writes. Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5 Reviewed-on: http://gerrit.cloudera.org:8080/12815 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M src/kudu/common/row_operations-test.cc M src/kudu/common/row_operations.cc M src/kudu/common/row_operations.h M src/kudu/master/catalog_manager.cc M src/kudu/tablet/tablet.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tserver/tablet_server-test.cc 7 files changed, 138 insertions(+), 43 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ib27c335e6a68336b88f75eb8fa2c45c6e18403d5 Gerrit-Change-Number: 12815 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2099: drop Java 7 support
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12816 ) Change subject: KUDU-2099: drop Java 7 support .. Patch Set 3: Code-Review+1 Agree it would be nice to use a few Java 8 things (eg Spliterator for scanners so it would be easy to parallelize scans from the Java client) -- To view, visit http://gerrit.cloudera.org:8080/12816 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I05fcea8bbc7b773f95429b324acd0b9d18a6e9f8 Gerrit-Change-Number: 12816 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 25 Mar 2019 16:17:58 + Gerrit-HasComments: No
[kudu-CR] KUDU-2750: Add create and alter timestamp to tables
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12842 ) Change subject: KUDU-2750: Add create and alter timestamp to tables .. Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master.proto@185 PS1, Line 185: // The create time of the table. nit: add "in seconds since the epoch" http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master.proto@186 PS1, Line 186: optional int64 create_time = 10; nit: can you rename this and the below field to '_timestamp' instead of '_time'? Just to make it extra clear that it's a point in time and not a duration http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master.proto@187 PS1, Line 187: // The last alter time of the table. same http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master_path_handlers.cc@202 PS1, Line 202: strCreateTime style nit: str_create_time http://gerrit.cloudera.org:8080/#/c/12842/1/src/kudu/master/master_path_handlers.cc@207 PS1, Line 207: std::string strAlterTime; same -- To view, visit http://gerrit.cloudera.org:8080/12842 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ife05719cea26b15fdf2b5667b60c3e89947cdda8 Gerrit-Change-Number: 12842 Gerrit-PatchSet: 1 Gerrit-Owner: helifu Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 25 Mar 2019 15:49:03 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2099: drop Java 7 support
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12816 ) Change subject: KUDU-2099: drop Java 7 support .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/12816 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I05fcea8bbc7b773f95429b324acd0b9d18a6e9f8 Gerrit-Change-Number: 12816 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 25 Mar 2019 13:48:53 + Gerrit-HasComments: No
