[kudu-CR] [security] avoid sparse seq numbers in TokenSigner

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/6329

to look at the new patch set (#3).

Change subject: [security] avoid sparse seq numbers in TokenSigner
..

[security] avoid sparse seq numbers in TokenSigner

Changed the way how TokenSigner assigns sequence numbers.  The new way
of assigning sequence numbers allows to avoid sparse sequence numbers
for the CheckNeedKey()-try-to-store-key-AddKey() sequence if
the 'try-to-store-key' part fails.

Added unit test for that and some other scenarios as well.

Change-Id: Ib84f3d4f2596b3e75d1e0132f1e3362812d5799e
---
M src/kudu/security/token-test.cc
M src/kudu/security/token_signer.cc
M src/kudu/security/token_signer.h
M src/kudu/security/token_signing_key.h
4 files changed, 273 insertions(+), 31 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/6329/3
-- 
To view, visit http://gerrit.cloudera.org:8080/6329
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib84f3d4f2596b3e75d1e0132f1e3362812d5799e
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] avoid sparse seq numbers in TokenSigner

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [security] avoid sparse seq numbers in TokenSigner
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6329/1/src/kudu/security/token-test.cc
File src/kudu/security/token-test.cc:

Line 117: TEST_F(TokenTest, TestTokenSignerAddKeyAfterImport) {
> As mentioned on slack, a test for the failure case in the commit message wo
Good catch!

Done: a new test added.


PS1, Line 146: valie
> valid
Done


http://gerrit.cloudera.org:8080/#/c/6329/1/src/kudu/security/token_signer.h
File src/kudu/security/token_signer.h:

PS1, Line 205: It's not a problem to call this method multiple times but call 
the AddKey()
 :   // method only once, effectively discarding all the generated 
keys except for
 :   // the key passed to the AddKey() call as a parameter. In 
other words,
 :   // it's possible and not a problem to have 'holes' in the key 
sequence
 :   // numbers. Other components working with verification of the 
signed tokens
 :   // should take that into account.
> Should this be updated?  I think the following sequence would now be proble
Right, this needs to be updated.

Yep, the former sequence should fail during the second AddKey() call.  The 
latter sequence should work with no issues.


-- 
To view, visit http://gerrit.cloudera.org:8080/6329
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib84f3d4f2596b3e75d1e0132f1e3362812d5799e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] avoid sparse seq numbers in TokenSigner

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/6329

to look at the new patch set (#2).

Change subject: [security] avoid sparse seq numbers in TokenSigner
..

[security] avoid sparse seq numbers in TokenSigner

Changed the way how TokenSigner assigns sequence numbers.  The new way
of assigning sequence numbers allows to avoid sparse sequence numbers
for the CheckNeedKey()-try-to-store-key-AddKey() sequence if
the 'try-to-store-key' part fails.

Added unit test for that and some other scenarios as well.

Change-Id: Ib84f3d4f2596b3e75d1e0132f1e3362812d5799e
---
M src/kudu/security/token-test.cc
M src/kudu/security/token_signer.cc
M src/kudu/security/token_signer.h
M src/kudu/security/token_signing_key.h
4 files changed, 270 insertions(+), 31 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/6329/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6329
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib84f3d4f2596b3e75d1e0132f1e3362812d5799e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP: Expose a way to set "advanced" non-types scan options

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: WIP: Expose a way to set "advanced" non-types scan options
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6624/4//COMMIT_MSG
Commit Message:

PS4, Line 15: without "polluting" the api and in such a way that we can remove
: support for this in the future.
> So if I may try and restate your argument:
Yep, good summary.

I would guess that many clients will be "badly behaved" if they can mandate the 
server version (eg Impala may decide it's better for them to just require Kudu 
1.4 rather than support back-compat, after weighing the cost of maintaining 
compat). I'm happy to leave that choice up to consumers.


-- 
To view, visit http://gerrit.cloudera.org:8080/6624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: corruptor test utility

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: log block manager: corruptor test utility
..


Patch Set 6: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/6582
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I197b693d5a3c1a909e91b772ebfb31e50bae488b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] fs: generate report during Open

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: fs: generate report during Open
..


Patch Set 7: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6581/4/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:

PS4, Line 69: FsReport report;
> Not sure where you got 120 from. The Google Style Guide (https://google.git
right 100, not 120. guess I didn't recall the mostly-80 recommendation.

Still see it as worthless since I can't really have a less than 100 cols editor 
given how often there are spills.

To keep from constantly scrolling horizontally in most parts of the code base I 
have at least 100 cols visible at which point vertical space is more important 
to me.

anyway, not worth blocking work for comment wrapping. my personal preference, 
if we're taking those into account, has been noted, moving on.


http://gerrit.cloudera.org:8080/#/c/6581/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

PS4, Line 584: ile_si
> But that's obvious from L585-588, isn't it?
I guess my personal preference is precision vs color. For me I would prefer a: 
"First verify that the record's offset/length aren't non-existent or negative." 
or somesuch than a " First verify that the record's offset/length aren't wildly 
incorrect." that forces me to go read the if conditions to understand what that 
means. again, moving on.


-- 
To view, visit http://gerrit.cloudera.org:8080/6581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc1d512e2bed103c8d41623a63b4d071532b35e
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Expose a way to set "advanced" non-types scan options

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: WIP: Expose a way to set "advanced" non-types scan options
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6624/4//COMMIT_MSG
Commit Message:

PS4, Line 15: without "polluting" the api and in such a way that we can remove
: support for this in the future.
> Yea, in this case (and in many similar cases) the flag is really just passe
So if I may try and restate your argument:
1. This option only makes sense if passed through directly to the server (I 
agree that implementing a compatibility layer doesn't make sense).
2. As such, an application making use of it must already contend with a "not 
supported" error should it contact an old server.
3. Thus, the presence of an additional "not supported" error when a future 
_client library_ doesn't support the option is not an imposition, because a 
well-behaved application will have handled it anyway as per #2.

I can buy that.


-- 
To view, visit http://gerrit.cloudera.org:8080/6624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Expose a way to set "advanced" non-types scan options

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: WIP: Expose a way to set "advanced" non-types scan options
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6624/4/src/kudu/client/scan_configuration.cc
File src/kudu/client/scan_configuration.cc:

PS4, Line 184:  if (strcasecmp(name.c_str(), kPadUnixTimeMicrosScanOption)) {
> yeah, I noticed, just trying to get feedback on the API. doesn't seem like 
yeah, I figured, though I'd like to get coding even if the API changes

I'm working around these issues for now


-- 
To view, visit http://gerrit.cloudera.org:8080/6624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Expose a way to set "advanced" non-types scan options

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: WIP: Expose a way to set "advanced" non-types scan options
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6624/4//COMMIT_MSG
Commit Message:

PS4, Line 15: without "polluting" the api and in such a way that we can remove
: support for this in the future.
> Likely a run-time error, as before. In any case I don't think we should the
Yea, in this case (and in many similar cases) the flag is really just passed 
down to the server, in which case it's perfectly likely that the server is 
allowed to reject it even if the client library has accepted it. So, the client 
can produce runtime "not-supported" errors, even if the consumer of the API 
linked against an appropriate client version.

Certainly the client could _in theory_ do a compatibility layer by which it 
notices that the server doesn't support the flag, and instead does a full 
memcpy/re-padding of the scanner data, so that it implements the requested 
format. However that would be at a very high perf cost, and I think it would 
probably be preferable to just tell the consumer that the format is not 
supported, rather than have a relatively-silent perf issue that might never be 
discovered.

If you want to take the Linux analogy, I'd say this is similar to ioctl() or 
setsockopt() or other various "more flexible" APIs where the feature may not be 
supported in a particular runtime environment (filesystem, network socket type, 
etc)

Another analogy to other software: the JDK has plenty of "undocumented" APIs 
which can be useful to advanced users who are willing to make the tradeoff that 
they might go away at some point. sun.misc.Unsafe is the most commonly-used 
example, but there are plenty of other cases where I've seen people downcast in 
order to do certain tricky things that are outside the scope of what the 
platform provider wants to generally support.


-- 
To view, visit http://gerrit.cloudera.org:8080/6624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Expose a way to set "advanced" non-types scan options

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: WIP: Expose a way to set "advanced" non-types scan options
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6624/4/src/kudu/tserver/tserver.proto
File src/kudu/tserver/tserver.proto:

Line 377:   // Whether the server supports padding UNIXTIME_MICROS slots.
> something doesn't seem hooked up correctly, when I play with this I'm getti
Yeah this isn't hooked up in TabletServiceImpl yet


-- 
To view, visit http://gerrit.cloudera.org:8080/6624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Expose a way to set "advanced" non-types scan options

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: WIP: Expose a way to set "advanced" non-types scan options
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6624/4/src/kudu/client/scan_configuration.cc
File src/kudu/client/scan_configuration.cc:

PS4, Line 184:  if (strcasecmp(name.c_str(), kPadUnixTimeMicrosScanOption)) {
> here and below, this isn't going to do what you want it to. strcasecmp retu
yeah, I noticed, just trying to get feedback on the API. doesn't seem like 
we're going this route anyway


-- 
To view, visit http://gerrit.cloudera.org:8080/6624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Expose a way to set "advanced" non-types scan options

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: WIP: Expose a way to set "advanced" non-types scan options
..


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6624/4/src/kudu/client/scan_configuration.cc
File src/kudu/client/scan_configuration.cc:

PS4, Line 184:  if (strcasecmp(name.c_str(), kPadUnixTimeMicrosScanOption)) {
here and below, this isn't going to do what you want it to. strcasecmp returns 
0 if they're equal


PS4, Line 186: pad_unixtime_micros_for_impala_
return OK after this and below, otherwise you'll return the error on l195


PS4, Line 187: true
false?

also compare is used wrong


http://gerrit.cloudera.org:8080/#/c/6624/4/src/kudu/tserver/tserver.proto
File src/kudu/tserver/tserver.proto:

Line 377:   // Whether the server supports padding UNIXTIME_MICROS slots.
something doesn't seem hooked up correctly, when I play with this I'm getting

Remote error: unsupported feature flags


-- 
To view, visit http://gerrit.cloudera.org:8080/6624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Expose a way to set "advanced" non-types scan options

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: WIP: Expose a way to set "advanced" non-types scan options
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6624/4//COMMIT_MSG
Commit Message:

PS4, Line 15: without "polluting" the api and in such a way that we can remove
: support for this in the future.
> Regarding this advanced option API, is an enum constant strong enough? I un
Likely a run-time error, as before. In any case I don't think we should the 
current structure (SetAdvancedScanOption) if we're only going to allow it to 
set boolean values through an enum. Much rather then having a proper method 
whose removal fails outright at both compile and link time.


-- 
To view, visit http://gerrit.cloudera.org:8080/6624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Expose a way to set "advanced" non-types scan options

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: WIP: Expose a way to set "advanced" non-types scan options
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6624/4//COMMIT_MSG
Commit Message:

PS4, Line 15: without "polluting" the api and in such a way that we can remove
: support for this in the future.
> Thanks for weighing in.
Regarding this advanced option API, is an enum constant strong enough? I 
understand how it will surface an API removal as a breakage at compile time, 
but if an existing application binary is used, will the removal manifest as a 
link time error or a run time error?

Regarding API development, I had drafted a response comparing the Linux syscall 
API to Guava and Hadoop's APIs, but I don't think it'll be a productive 
discussion. It's clear to me that I would prefer if unstable APIs in Kudu were 
highly discouraged if not prohibited outright, and if Kudu's API culture were 
more egalitarian (i.e. consumer "weight" didn't play a role in dictating 
exceptions from the "no unstable APIs" rule). But, I appear to be in the 
minority, and this isn't the right forum for that discussion. So consider it 
dropped.


-- 
To view, visit http://gerrit.cloudera.org:8080/6624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Expose a way to set "advanced" non-types scan options

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: WIP: Expose a way to set "advanced" non-types scan options
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6624/4//COMMIT_MSG
Commit Message:

PS4, Line 15: without "polluting" the api and in such a way that we can remove
: support for this in the future.
> I agree with the first portion of the soap box - I would prefer an API that
Thanks for weighing in.

I have nothing against the enum, personally, just seems limiting from an API 
perspective as it doesn't allow to set a value and thus reduces the usefulness 
of the api itself. for instance say that we would want in the future to enable 
users to set the size of the row block for scans, this would need a value.

If we're going the enum route maybe we should just have 
"pad_timestamp_16_bytes" getters and setters in KuduScanner and be done with it.

Regarding the second part of the soapbox, I agree with todd. As long as it's 
pretty clear what is the api contract, from an evolution perspective, then 
users can choose to take in the risk or not. Finally I would say that while we 
might not easily accept these kinds from any random contributor I would say we 
would definitely consider them seriously for any big Kudu consumer, be it 
Impala or Spark or any other well known compute/query layer.


-- 
To view, visit http://gerrit.cloudera.org:8080/6624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: use extent maps to decide whether to truncate containers

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: log block manager: use extent maps to decide whether to 
truncate containers
..


Patch Set 6:

Have you done any tests of how this affects startup time, particularly after a 
full drop_caches on a server with a large number of containers/extents? Am 
worried that we might regress startup time significantly

-- 
To view, visit http://gerrit.cloudera.org:8080/6585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3541f70721328d5af911f4889f8b4bcf0de3ebf8
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP: Expose a way to set "advanced" non-types scan options

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: WIP: Expose a way to set "advanced" non-types scan options
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6624/4//COMMIT_MSG
Commit Message:

PS4, Line 15: without "polluting" the api and in such a way that we can remove
: support for this in the future.
> I am opposed to introducing an API that's weakly structured in this way.
I agree with the first portion of the soap box - I would prefer an API that 
uses named enum constants, like:

SetAdvancedOption(PAD_TIMESTAMP_16_BYTES);

so that we get the benefits of type safety, documentation, etc.


On the second part of the soapbox, I don't think we should constrain ourselves 
that all APIs need to have an equal stability/compatibility guarantee. As I 
think we all agree, maintaining a stable API is costly since it involves 
testing, generates back-compat code concerns, etc. That's why we always try to 
make the APIs as narrow as possible, think through them up front, etc.

That said, I think there's a case to be made that there are some advanced APIs 
where we can save a lot of cost (docs, compat, etc) by signing up for a lighter 
compatibility contract. It does force consumers to make the choice to tighten 
the compatibility matrix (eg Impala 2.9 would only support Kudu 1.4 or 
whatever), but so long as this restriction is advertised up front, I think 
that's OK. The consumer can then weigh the advantages of using the API (in this 
case, performance) against the disadvantages (having to tightly couple 
versions).

So the remaining question is what the decision framework is for whether an API 
should be stable/documented/etc vs unstable/etc. I think for me it comes down 
to how many consumers we expect to have, and how advanced they are. In the case 
of a query engine like Impala building an integration with Kudu, we already 
expect them to be closely following Kudu development (e.g every release we add 
new predicate types which Impala adds pushdown for). And we expect them to be 
very advanced consumers (they already rely on the byte format of our cells to 
get good performance). So adding one more advanced API that they need to dig 
into the code a bit to use isn't problematic in my view.

Meanwhile, users who are more concerned with ease of use and version 
flexibility (probably the vast majority of users) can stick with our nice 
documented stable API. Even if we documented and provided guarantees for this 
new one, I doubt most users would want to use it.

Thoughts?


-- 
To view, visit http://gerrit.cloudera.org:8080/6624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [mini-kdc] adapted config for krb5 1.15

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [mini-kdc] adapted config for krb5 1.15
..


Patch Set 2:

Unrelated error due to change in error message -- will address that separately.

-- 
To view, visit http://gerrit.cloudera.org:8080/6629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie793b742c2d41f935178f3f4da7a0d0923f79816
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [mini-kdc] adapted config for krb5 1.15

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [mini-kdc] adapted config for krb5 1.15
..


Patch Set 2: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/6629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie793b742c2d41f935178f3f4da7a0d0923f79816
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] log block manager: use extent maps to decide whether to truncate containers

[Adar Dembo (Code Review)]

Hello David Ribeiro Alves, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/6585

to look at the new patch set (#6).

Change subject: log block manager: use extent maps to decide whether to 
truncate containers
..

log block manager: use extent maps to decide whether to truncate containers

This patch wires the new GetExtentMap() method into the LBM for use in
determining whether there exists allocated disk space beyond the container
data file's logical size. Why bother? Well, it's not possible for the
"full container preallocation" accounting in FsReport to be accurate
otherwise. Also, reducing the number of cases wherein containers are
truncated ought to be safer. Finally, we'll eventually use the extent map
for other purposes (e.g. repunching holes efficiently at startup).

My first implementation required LBM users to have GetExtentMap()-compatible
systems. Testing showed this to be safe across Kudu's supported distro and
filesystem matrix. The notable exception was aufs, used by Docker containers
that run our pre-commit tests. It would be unfortunate if Kudu were unusable
in Docker [1], so I adjusted the behavior to be more forgiving.

I also snuck in some cosmetic changes to a few error messages.

1. Though isn't KUDU-1419 still an issue? Maybe not in all versions of aufs?

Change-Id: I3541f70721328d5af911f4889f8b4bcf0de3ebf8
---
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
3 files changed, 118 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/6585/6
-- 
To view, visit http://gerrit.cloudera.org:8080/6585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3541f70721328d5af911f4889f8b4bcf0de3ebf8
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] fs: generate report during Open

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: fs: generate report during Open
..


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6581/4/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:

PS4, Line 69: // unnecessarily.
> I don't think that it makes a lot of sense for people to have different wra
Not sure where you got 120 from. The Google Style Guide 
(https://google.github.io/styleguide/cppguide.html#Line_Length) mandates 80. 
Our style guide relaxes that to 100 (and that's enforced by LINT precommit 
runs), but recommends staying under 80 
(http://kudu.apache.org/docs/contributing.html#_line_length).


http://gerrit.cloudera.org:8080/#/c/6581/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

PS4, Line 324: truly catastrophic
> can you spell that out then? it's unlikely that the next person using this 
Done


PS4, Line 584: wildly
> can you spell that out then, please? (that's kind of what I meant from my c
But that's obvious from L585-588, isn't it?

This comment isn't here to explain the checks in detail (since that's obvious 
from the code), but to contrast with the comment on L594. That is, after 
reading L594, you should know that it's only OK to compare the data_file_size 
with the record offset+length thanks to the checks on L585-588, and if you were 
modifying this function, you shouldn't reorder the comparison to precede those 
checks.


PS4, Line 1726: Truncation is performed even if the container's logical file 
size
  :   // (available by proxy via preallocated_offset_) and 
total_bytes_written_
  :   // agree, because XFS's speculative preallocation feature 
may artificially
  :   // enlarge the file without updating its file size.
> Yeah, but if it starts with "For XFS filesystems" or somesuch, then you kno
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc1d512e2bed103c8d41623a63b4d071532b35e
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] fs: generate report during Open

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/6581

to look at the new patch set (#7).

Change subject: fs: generate report during Open
..

fs: generate report during Open

This patch modifies the FsManager and block managers to produce a filesystem
report when opened. The report includes various filesystem statistics as
well as all inconsistencies found and repaired.

The heart of the patch is the FsReport nested data structure. Originally
implemented as a protobuf to take advantage of DebugString(), I found it to
be a poor fit once I began customizing the log output, so I rewrote it in
its current form. The report includes fs-wide statistics as well as optional
consistency checks that may or may not be performed by the block managers.
Reports can be merged; the LBM takes advantage of this as it processes data
directories in parallel. The consistency checks have structure, so as to
simplify testing and ToString() customization. A sample report can be found
at the end of this commit message.

Thanks to the level of detail in the FsReport, the LBM now separates the act
of finding inconsistencies from repairing them. This makes it much easier to
enforce that repairs aren't done on a read-only filesystem, or if there were
any fatal and irreparable errors.

Thorough testing of the inconsistencies in the report is deferred to a
different patch as this one was already quite large.

Other changes of note:
- The LBM treats data files without matching metadata files as incomplete
  containers; previously only metadata files without matching data files
  were treated in this way.
- The LBM maps all containers by name so they can be looked up during the
  repair process; previously they were stored in a vector.
- The checks performed by "kudu fs check" are in FsReport. I think this
  makes sense, and it's an acknowledgement that they should be performed by
  the block manager in the future. The code in tool_action_fs.cc was
  restructured to write its findings into the report before logging it.

Block manager report

1 data directories: 
/tmp/kudutest-1000/block_manager-stress-test.BlockManagerStressTest_1.StressTest.1491963221016069-10059
Total live blocks: 47
Total live bytes: 221524
Total live bytes (after alignment): 323582
Total number of LBM containers: 53 (21 full)
Did not check for missing blocks
Did not check for orphaned blocks
Total full LBM containers with preallocated space: 0 (0 repaired)
Total full LBM container preallocated space in bytes: 0 (0 repaired)
Total incomplete LBM containers: 3 (3 repaired)
Misaligned block in container 
/tmp/kudutest-1000/block_manager-stress-test.BlockManagerStressTest_1.StressTest.1491963221016069-10059/038a826c27db4e6da8237ce79853ece2:
 block_id {
  id: 9225972546088407965
}
op_type: CREATE
timestamp_us: 0
offset: 1052673
length: 16384

Misaligned block in container 
/tmp/kudutest-1000/block_manager-stress-test.BlockManagerStressTest_1.StressTest.1491963221016069-10059/0612d7730bdc4cfd83afbaed271d5289:
 block_id {
  id: 3148617980286357277
}
op_type: CREATE
timestamp_us: 0
offset: 1048577
length: 16384

Total LBM partial records: 7 (7 repaired)

Change-Id: Idcc1d512e2bed103c8d41623a63b4d071532b35e
---
M src/kudu/fs/CMakeLists.txt
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
A src/kudu/fs/fs_report.cc
A src/kudu/fs/fs_report.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/server/server_base.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
M src/kudu/util/pb_util.cc
17 files changed, 1,092 insertions(+), 216 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/6581/7
-- 
To view, visit http://gerrit.cloudera.org:8080/6581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idcc1d512e2bed103c8d41623a63b4d071532b35e
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] fs: generate report during Open

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: fs: generate report during Open
..


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6581/4/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:

PS4, Line 69: // unnecessarily.
> Generally I wrap at 80 characters, and this extends to 81 if unwrapped. But
I don't think that it makes a lot of sense for people to have different 
wrapping policies. Not saying that 80 chars is wrong or right, just that we 
should all do it or none of us should do it. Given that code style mandates 
wrapping at 120 I usually point as nits forced wrapping well below that where 
there's a single word in the last line. particularly when wrapping occurs in 
places where you can find surrounding code that doesn't, like this one.

In general, if devs need to have their displays adjusted to 120 cols anyway, 
vertical space is more precious than horizontal space, IMO


http://gerrit.cloudera.org:8080/#/c/6581/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

PS4, Line 324: truly catastrophic
> This isn't for fatal and irreparable inconsistencies though; this is for "w
can you spell that out then? it's unlikely that the next person using this will 
know what you mean by "truly catastrophic"


PS4, Line 584: wildly
> Non-existent or negative values (what we're testing for in the subsequent c
can you spell that out then, please? (that's kind of what I meant from my 
comment)


PS4, Line 1726: Truncation is performed even if the container's logical file 
size
  :   // (available by proxy via preallocated_offset_) and 
total_bytes_written_
  :   // agree, because XFS's speculative preallocation feature 
may artificially
  :   // enlarge the file without updating its file size.
> Why? It's just one sentence and you can't read it without seeing "XFS specu
Yeah, but if it starts with "For XFS filesystems" or somesuch, then you know 
you don't need to read it at all if what you're concerned about is another 
filesystem


PS4, Line 1753: // TODO(adar): track as an inconsistency?
> I hesitated to track it as an inconsistency for it for a couple reasons:
sounds good


-- 
To view, visit http://gerrit.cloudera.org:8080/6581
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idcc1d512e2bed103c8d41623a63b4d071532b35e
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log block manager: use extent maps to decide whether to truncate containers

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: log block manager: use extent maps to decide whether to 
truncate containers
..


Patch Set 5: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/6585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3541f70721328d5af911f4889f8b4bcf0de3ebf8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] env: add RWFile::GetExtentMap for analyzing file extents

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: env: add RWFile::GetExtentMap for analyzing file extents
..


Patch Set 5: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/6583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I35bd1bdb9e1a839af2ab95ea73b79217c1f4a2b3
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] log block manager: corruptor test utility

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: log block manager: corruptor test utility
..


Patch Set 5: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/6582
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I197b693d5a3c1a909e91b772ebfb31e50bae488b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [security] avoid sparse seq numbers in TokenSigner

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [security] avoid sparse seq numbers in TokenSigner
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6329/1/src/kudu/security/token-test.cc
File src/kudu/security/token-test.cc:

Line 117: TEST_F(TokenTest, TestTokenSignerAddKeyAfterImport) {
As mentioned on slack, a test for the failure case in the commit message would 
be good.


PS1, Line 146: valie
valid


http://gerrit.cloudera.org:8080/#/c/6329/1/src/kudu/security/token_signer.h
File src/kudu/security/token_signer.h:

PS1, Line 205: It's not a problem to call this method multiple times but call 
the AddKey()
 :   // method only once, effectively discarding all the generated 
keys except for
 :   // the key passed to the AddKey() call as a parameter. In 
other words,
 :   // it's possible and not a problem to have 'holes' in the key 
sequence
 :   // numbers. Other components working with verification of the 
signed tokens
 :   // should take that into account.
Should this be updated?  I think the following sequence would now be 
problematic:

CheckNeedKey
CheckNeedKey
AddKey
AddKey

as opposed to this sequence, which should be fine:

CheckNeedKey
AddKey
CheckNeedKey
AddKey


-- 
To view, visit http://gerrit.cloudera.org:8080/6329
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib84f3d4f2596b3e75d1e0132f1e3362812d5799e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [mini-kdc] adapted config for krb5 1.15

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [mini-kdc] adapted config for krb5 1.15
..


Patch Set 2: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/6629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie793b742c2d41f935178f3f4da7a0d0923f79816
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [mini-kdc] adapted config for krb5 1.15

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/6629

to look at the new patch set (#2).

Change subject: [mini-kdc] adapted config for krb5 1.15
..

[mini-kdc] adapted config for krb5 1.15

Starting with version 1.15 of Kerberos5, the krb5kdc by default listens
on TCP ports as well if not specified otherwise.  However, the mini-kdc
logic assumes the daemon listens only on UDP ports.  Explicit setting of
the kdc_tcp_ports configuration property to an empty string in krb.conf
disables opening of the TCP ports by krb5kdc.

Change-Id: Ie793b742c2d41f935178f3f4da7a0d0923f79816
---
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKdc.java
M src/kudu/security/test/mini_kdc.cc
2 files changed, 2 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/6629/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie793b742c2d41f935178f3f4da7a0d0923f79816
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [mini-kdc] adapted config for krb5 1.15

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [mini-kdc] adapted config for krb5 1.15
..


Patch Set 1:

> Do we need this in the Java kdc also?

Ugh, sure.  I haven't run Java tests since upgrading my MacPorts.

-- 
To view, visit http://gerrit.cloudera.org:8080/6629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie793b742c2d41f935178f3f4da7a0d0923f79816
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [mini-kdc] adapted config for krb5 1.15

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [mini-kdc] adapted config for krb5 1.15
..


Patch Set 1:

Do we need this in the Java kdc also?

-- 
To view, visit http://gerrit.cloudera.org:8080/6629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie793b742c2d41f935178f3f4da7a0d0923f79816
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP: KUDU-463. Add checksumming to cfile

[Grant Henke (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/6630

to look at the new patch set (#2).

Change subject: WIP: KUDU-463. Add checksumming to cfile
..

WIP: KUDU-463. Add checksumming to cfile

Adds optional checksumming and validation to cfile blocks.

Introduces 2 flags to control cfile checksumming:
- cfile_write_checksums (default false)
- cfile_verify_checksums (default true)

cfile_write_checksums is used in the CFileWriter to enable computing and
appending Crc32 checksums to the end of each cfile block.

cfile_write_checksums is defaulted to false to ensure upgrades don't
immeditaly result in performance degredation and incompatible data on
downgrade. It can and likely should be defaulted to true in a later release.

When cfile_write_checksums is set to true, the existing incompatible_features
bitset in the cfile footer is used. A "checksum" bit is set to ensure a clear
error message when older versions of Kudu try to read the file. If checksums
are not writen the incompatible_features "checksum" bit is not set.

cfile_verify_checksums is used in the CFileReader to enabled validating the
data against the written checksum. cfile_verify_checksums is defaulted to
true since validation only occurs if checksums are written. Any data that
was written before checksumming was an option or when cfile_write_checksums
was false will not be verified.

TODO:
- More unit and perf testing
- Change cfile block to page to avoid term overload?

Change-Id: I6756834cd7f27af258797a3654a95244abeb0976
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_util.h
M src/kudu/cfile/cfile_writer.cc
4 files changed, 102 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/6630/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6630
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] WIP: KUDU-463. Add checksumming to cfile

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change.

Change subject: WIP: KUDU-463. Add checksumming to cfile
..


Patch Set 1:

This is just a quick push to sanity check my approach.

-- 
To view, visit http://gerrit.cloudera.org:8080/6630
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] WIP: KUDU-463. Add checksumming to cfile

[Grant Henke (Code Review)]

Grant Henke has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/6630

Change subject: WIP: KUDU-463. Add checksumming to cfile
..

WIP: KUDU-463. Add checksumming to cfile

Adds optional checksumming and validation to cfile blocks.

Introduces 2 flags to control cfile checksumming:
- cfile_write_checksums (default false)
- cfile_verify_checksums (default true)

cfile_write_checksums is used in the CFileWriter to enable computing and
appending Crc32 checksums to the end of each cfile block.

cfile_write_checksums is defaulted to false to ensure upgrades don't
immeditaly result in performance degredation and incompatible data on
downgrade. It can and likely should be defaulted to true in a later release.

When cfile_write_checksums is set to true, the existing incompatible_features
bitset in the cfile footer is used. A "checksum" bit is set to ensure a clear
error message when older versions of Kudu try to read the file. If checksums
are not writen the incompatible_features "checksum" bit is not set.

cfile_verify_checksums is used in the CFileReader to enabled validating the
data against the written checksum. cfile_verify_checksums is defaulted to
true since validation only occurs if checksums are written. Any data that
was written before checksumming was an option or when cfile_write_checksums
was false will not be verified.

TODO:
- More unit and perf testing
- Change cfile block to page to avoid term overload?

Change-Id: I6756834cd7f27af258797a3654a95244abeb0976
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_util.h
M src/kudu/cfile/cfile_writer.cc
4 files changed, 103 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/6630/1
-- 
To view, visit http://gerrit.cloudera.org:8080/6630
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6756834cd7f27af258797a3654a95244abeb0976
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 

[kudu-CR] [mini-kdc] adapted config for krb5 1.15

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/6629

Change subject: [mini-kdc] adapted config for krb5 1.15
..

[mini-kdc] adapted config for krb5 1.15

Starting with version 1.15 of Kerberos5, the krb5adm by default listens
on TCP ports as well if not specified otherwise.  However, the mini-kdc
logic assumes the daemon listens only on UDP ports.  Explicit setting of
the kdc_tcp_ports configuration property to an empty string in krb.conf
disables opening TCP ports.

Change-Id: Ie793b742c2d41f935178f3f4da7a0d0923f79816
---
M src/kudu/security/test/mini_kdc.cc
1 file changed, 1 insertion(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/29/6629/1
-- 
To view, visit http://gerrit.cloudera.org:8080/6629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie793b742c2d41f935178f3f4da7a0d0923f79816
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 

[kudu-CR] Allow to release an rpc transfer's data

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Allow to release an rpc transfer's data
..


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6592/8/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

Line 327:   Status ReleaseTransferData(uint8_t** released_data);
why not make this a unique_ptr* so that it is more explicitly 
transferring ownership, and ensures that the caller uses the right delete[] 
variant?


http://gerrit.cloudera.org:8080/#/c/6592/8/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:

Line 200:   // Deleting 'response_buffer' must be done through "delete []".
same, using unique_ptr would ensure this


-- 
To view, visit http://gerrit.cloudera.org:8080/6592
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I55c2b2d24c347ccdffc054eeb4131eaf9e82f901
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Expose a way to set "advanced" non-types scan options

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: WIP: Expose a way to set "advanced" non-types scan options
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6624/4//COMMIT_MSG
Commit Message:

PS4, Line 15: without "polluting" the api and in such a way that we can remove
: support for this in the future.
I am opposed to introducing an API that's weakly structured in this way.

Generally speaking, string-based key/value interfaces like this are attractive 
to library developers because the perceived cost of adding or removing an API 
is low. In reality, these APIs are actually more hostile to application 
developers than their strongly typed equivalents, for several reasons:
1. It's generally harder to figure out what the legal combinations of keys and 
values are. That is, it's harder to sift through documentation and/or string 
constants than through a set of well defined and strongly typed functions.
2. If an application tries to use an API that has been removed, the error will 
surface at run time instead of link (or compile) time.

I don't think removing a strongly typed API is any easier or harder than an API 
like this one: in either case, there must be a well-defined and well-publicized 
period of deprecation followed by the removal of the API.

Now, since I appear to be standing on a soap box, I feel compelled to continue 
my ranting. I appreciate that Impala is one of a small set of major 
applications that use Kudu, and as such, I also appreciate that many of our new 
APIs are driven Impala's use cases. But, I'm getting tired of seeing APIs 
defined in a sorta-hidden or you-need-to-know-the-undocumented-format kind of 
way. We wouldn't tolerate short cuts like these if they were proposed on behalf 
of github.com/someguy/mycooldataapp; we shouldn't tolerate them for a mature 
application like Impala either.

Moreover, we can't assume that users will always deploy Impala and Kudu in some 
predefined lockstep configuration; these are both Apache projects and users 
will try whatever combinations they see fit. As such, when we add APIs for 
Impala, we should intend for them to last forever like any of our APIs, and we 
should put enough forethought into them to stand by that commitment.


-- 
To view, visit http://gerrit.cloudera.org:8080/6624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Expose a way to set "advanced" non-types scan options

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/6624

to look at the new patch set (#4).

Change subject: WIP: Expose a way to set "advanced" non-types scan options
..

WIP: Expose a way to set "advanced" non-types scan options

This adds a new method to KuduScanner that allows to set
"advanced" scan options just by providing a pair of key
and value strings.

The main target for this feature is to allow Impala scanners
to specify that padding is required for UNIXTIME_MICROS columns
without "polluting" the api and in such a way that we can remove
support for this in the future.

This api could potentially be used to set "advanced" scanner hints
in the future.

WIP: because this needs a directed end-to-end test, even though
the wire-protocol itself has been tested in a previous patch.

Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scanner-internal.cc
M src/kudu/tserver/scanners.cc
M src/kudu/tserver/scanners.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
9 files changed, 101 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/6624/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6624
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I043b6514dc5fc307fc9c94eb41f3ae79796ba273
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon