date:20180829

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11348 )

Change subject: build: fix run-test.sh when not retrying all tests
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11348/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11348/2//COMMIT_MSG@13
PS2, Line 13:   line 77: [: : integer expression expected
> Was this an actual failure? Or just a warning?
I think it was a log that didn't necessarily fail since we're still reporting 
failures to the dashboard, so the rest of run-test.sh must be running. That 
said, this may have broken retries.


http://gerrit.cloudera.org:8080/#/c/11348/2/build-support/run-test.sh
File build-support/run-test.sh:

http://gerrit.cloudera.org:8080/#/c/11348/2/build-support/run-test.sh@80
PS2, Line 80:   if [ -f "$KUDU_FLAKY_TEST_LIST" ] && [ $(grep --count 
--line-regexp "$SHORT_TEST_NAME" "$KUDU_FLAKY_TEST_LIST") -ne 0 ]; then
> We don’t actually use the grep result; could we drop it, add -q, and take t
Neat, done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb208194fed255361b2987e945ef91779742d4d7
Gerrit-Change-Number: 11348
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 29 Aug 2018 19:14:23 +
Gerrit-HasComments: Yes

[kudu-CR](gh-pages) Blogpost describing index skip scan optimization.

Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11263 )

Change subject: Blogpost describing index skip scan optimization.
..

Patch Set 4:

(16 comments)

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

http://gerrit.cloudera.org:8080/#/c/11263/4//COMMIT_MSG@6
PS4, Line 6:
: Blogpost describing index skip scan optimization.
:
In reviewing blogposts, it's generally helpful to post a link to a rendered
version, e.g. posting to your own github, which will automatically render the
*.md

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md
File _posts/2018-08-17-index-skip-scan-optimization-in-kudu.md:

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@9
PS4, Line 9: does not contain the first column of the composite (multi-column)
primary key.
This already seems like it's going a bit too far into implementation details.
Maybe instead note something like: 'I optimized the Kudu scan-path by
implementing a technique called an "index-skip scan."'

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@11
PS4, Line 11: Example
: ==
Probably don't need this.

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@14
PS4, Line 14:
What do these do?

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@31
PS4, Line 31: as B-Tree
nit: "a B-tree", and no need to capitalize "Tree" below

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@33
PS4, Line 33: ("host")
nit: perhaps using ``s would be more reasonable here (ie. `host`). Then it'd be
formatted as monospace font. Here and elsewhere

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@32
PS4, Line 32: The data is sorted lexicographically starting from the leftmost
primary key column and stored in the B-Tree leaf nodes.
: Therefore, when the user query contains the first key column
("host"), Kudu uses the primary key range push down
: operation to optimize the scan time.
IMO this doesn't convey the idea that the data is sorted by the composite of
all primary key columns. Also not sure what you mean by "primary key range push
down operation".

Also, overall for this project, I think it's always been helpful to
think/reason about it with some example data. I think having an dummy dataset
of a handful of rows with a decent number of prefix keys would make this
blogpost more understandable to the layperson. It'd also serve as a concrete
example of why we can't use the PK index if the predicate doesn't contain the
first key.

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@39
PS4, Line 39:
nit: here and elsewhere, no need for spaces before punctuation marks

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@46
PS4, Line 46: form a prefix.
The crux of this is the prefixes are also sorted, and all rows of a given
prefix are also sorted by the remaining PK columns. A prefix with no other
properties isn't necessarily useful, so without calling that out, it might be
hard to see why having these prefixes are helpful.

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@51
PS4, Line 51: For example, consider the query :
: {% highlight SQL %}
: select clusterid from metrics where tstamp = 100;
: {% endhighlight %}
:
: ![png]({{ site.github.url
}}/img/index-skip-scan/skip-scan-example-table.png){:height="500px"
width="500px" .img-responsive}
: *Sample rows of Table "metrics" (sorted by key columns for
simplicity).*
Ah, so you _do_ have an example! I think it'd be helpful setting this up up
front, saying, here is how data is organized in Kudu today, and based on that,
why it's not straightforward to use the index when there aren't predicates on
the first primary key, etc.

Also isn't the data _actually_ stored like this? I.e. not for simplicity, but
this actually represents how Kudu would see the data, doesn't it?

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@61
PS4, Line 61: host = "helium"
nit: backticks here too and everywhere else that has code snippets

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@62
PS4, Line 62: popularly known as index skip scan optimization can skip all the
rows for which host = "helium" and tstamp != 100
nit: it's great to get to the point that we cal

[kudu-CR] build: fix run-test.sh when not retrying all tests

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11348 )

Change subject: build: fix run-test.sh when not retrying all tests
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11348/3/build-support/run-test.sh
File build-support/run-test.sh:

http://gerrit.cloudera.org:8080/#/c/11348/3/build-support/run-test.sh@80
PS3, Line 80:   if [ -f "$KUDU_FLAKY_TEST_LIST" ] && [ $(grep --count 
--line-regexp "$SHORT_TEST_NAME" "$KUDU_FLAKY_TEST_LIST") -ne 0 ]; then
I think you can simplify further:

  if [ -f "$KUDU_FLAKY_TEST_LIST" ] && grep -q --line-regexp "$SHORT_TEST_NAME" 
"$KUDU_FLAKY_TEST_LIST"; then
...
  fi



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb208194fed255361b2987e945ef91779742d4d7
Gerrit-Change-Number: 11348
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 29 Aug 2018 20:21:57 +
Gerrit-HasComments: Yes

[kudu-CR] build: fix run-test.sh when not retrying all tests

Hello Kudu Jenkins, Adar Dembo,

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

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

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

Change subject: build: fix run-test.sh when not retrying all tests
..

build: fix run-test.sh when not retrying all tests

This is a follow-up to 5d69deb36925113796cd69f51061b8396b0174fc to clean
up some bash syntax. Notably, if KUDU_RETRY_ALL_FAILED_TESTS is unset,
the following message would be logged to stderr:

  line 77: [: : integer expression expected

Here is a dist-test run with KUDU_RETRY_ALL_FAILED_TESTS unset:
http://dist-test.cloudera.org/job?job_id=awong.1535506444.68746#

...and here is one with it set:
http://dist-test.cloudera.org/job?job_id=awong.1535506280.65606

Change-Id: Icb208194fed255361b2987e945ef91779742d4d7
---
M build-support/run-test.sh
1 file changed, 11 insertions(+), 13 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icb208194fed255361b2987e945ef91779742d4d7
Gerrit-Change-Number: 11348
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] build: fix run-test.sh when not retrying all tests

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11348 )

Change subject: build: fix run-test.sh when not retrying all tests
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11348/3/build-support/run-test.sh
File build-support/run-test.sh:

http://gerrit.cloudera.org:8080/#/c/11348/3/build-support/run-test.sh@80
PS3, Line 80:   if [ -f "$KUDU_FLAKY_TEST_LIST" ] && grep -q --count 
--line-regexp "$SHORT_TEST_NAME" "$KUDU_FLAKY_TEST_LIST"; then
> I think you can simplify further:
Doh, didn't add the changes. Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb208194fed255361b2987e945ef91779742d4d7
Gerrit-Change-Number: 11348
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 29 Aug 2018 20:34:01 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-428: add Sentry to thirdparty, mini-sentry

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11347 )

Change subject: KUDU-428: add Sentry to thirdparty, mini-sentry
..


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/11347/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11347/2//COMMIT_MSG@13
PS2, Line 13: - Stripped Sentry packaging. I've put an unmodified version of 
Sentry
:   2.0.1 into thirdparty. It weighs in at almost 200MiB and takes 
about
:   5s to startup on my laptop. We will probably want to add a 
stripped
:   version later to reduce both of these.
The Sentry integration isn't in any particular hurry, right? Maybe we can 
frontload this work, to save download time and space for Kudu developers who 
don't otherwise care about Sentry?


http://gerrit.cloudera.org:8080/#/c/11347/2/build-support/run_dist_test.py
File build-support/run_dist_test.py:

http://gerrit.cloudera.org:8080/#/c/11347/2/build-support/run_dist_test.py@153
PS2, Line 153: apache-sentry-*
This will change when you repackage the Sentry package, right?


http://gerrit.cloudera.org:8080/#/c/11347/2/src/kudu/sentry/mini_sentry.h
File src/kudu/sentry/mini_sentry.h:

http://gerrit.cloudera.org:8080/#/c/11347/2/src/kudu/sentry/mini_sentry.h@47
PS2, Line 47:   // Stops the mini Sentry service.
:   Status Stop() WARN_UNUSED_RESULT;
:
:   // Pause the Sentry service.
:   Status Pause() WARN_UNUSED_RESULT;
:
:   // Unpause the Sentry service.
:   Status Resume() WARN_UNUSED_RESULT;
At some point we should consider inserting MiniSentry, MiniHMS, and MiniKDC 
into the ExternalDaemon class hierarchy. At they very least they could share 
common Stop/Pause/Resume implementations.


http://gerrit.cloudera.org:8080/#/c/11347/2/src/kudu/sentry/mini_sentry.cc
File src/kudu/sentry/mini_sentry.cc:

http://gerrit.cloudera.org:8080/#/c/11347/2/src/kudu/sentry/mini_sentry.cc@142
PS2, Line 142:   
 : sentry.service.security.mode
 : none
 :   
This one isn't explained above, but that's because it's temporary until 
Kerberos support lands?


http://gerrit.cloudera.org:8080/#/c/11347/2/src/kudu/sentry/sentry_client-test.cc
File src/kudu/sentry/sentry_client-test.cc:

http://gerrit.cloudera.org:8080/#/c/11347/2/src/kudu/sentry/sentry_client-test.cc@46
PS2, Line 46: TEST_F(SentryClientTest, ItWorks) {
:   SentryClient client;
:   std::move(client);
: }
As useful as this was, you can remove it now.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I03f39cf9b2c813c0c305d085e1ad3851636326f5
Gerrit-Change-Number: 11347
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 29 Aug 2018 20:42:11 +
Gerrit-HasComments: Yes

[kudu-CR] build: fix run-test.sh when not retrying all tests

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11348 )

Change subject: build: fix run-test.sh when not retrying all tests
..


Patch Set 4: Code-Review+2

If you haven't already, could you manually test the path that runs through that 
grep statement just to make sure it works properly? If for some reason it 
always fails, we'll just not retry some tests and may not notice.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb208194fed255361b2987e945ef91779742d4d7
Gerrit-Change-Number: 11348
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 29 Aug 2018 20:43:30 +
Gerrit-HasComments: No

[kudu-CR] KUDU-428: add Sentry to thirdparty, mini-sentry

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11347 )

Change subject: KUDU-428: add Sentry to thirdparty, mini-sentry
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11347/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11347/2//COMMIT_MSG@22
PS2, Line 22: The mini Sentry is not yet configured with the location of the 
HMS,
:   which will be necessary to do anything non-trivial with it.
One thing I realized after putting this patch up is that there's a circular 
dependency between the HMS and Sentry in terms of configuring each with the 
other's address, including port.  The way our mini servers have worked up till 
this point is that the process is started with configuration to bind to port 0, 
then the actual port is discovered by polling lsof.  This isn't going to work 
when we have such a circular dependency, so we need to figure out what to do 
about that.


http://gerrit.cloudera.org:8080/#/c/11347/2/build-support/run_dist_test.py
File build-support/run_dist_test.py:

http://gerrit.cloudera.org:8080/#/c/11347/2/build-support/run_dist_test.py@153
PS2, Line 153: apache-sentry-*
> This will change when you repackage the Sentry package, right?
Correct.


http://gerrit.cloudera.org:8080/#/c/11347/2/src/kudu/sentry/mini_sentry.cc
File src/kudu/sentry/mini_sentry.cc:

http://gerrit.cloudera.org:8080/#/c/11347/2/src/kudu/sentry/mini_sentry.cc@142
PS2, Line 142:   
 : sentry.service.security.mode
 : none
 :   
> This one isn't explained above, but that's because it's temporary until Ker
Yeah, eventually this will be a configurable parameter.  I'm not sure at this 
point the extent of what this config flag does, other than Sentry doesn't 
complain about missing kerberos configuration at startup when it's set to 
'none'.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I03f39cf9b2c813c0c305d085e1ad3851636326f5
Gerrit-Change-Number: 11347
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 29 Aug 2018 20:49:03 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-428: add Sentry to thirdparty, mini-sentry

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11347 )

Change subject: KUDU-428: add Sentry to thirdparty, mini-sentry
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11347/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11347/2//COMMIT_MSG@22
PS2, Line 22: The mini Sentry is not yet configured with the location of the 
HMS,
:   which will be necessary to do anything non-trivial with it.
> One thing I realized after putting this patch up is that there's a circular
So it's impossible to use ephemeral ports for both? Do either allow already 
bound ports to be transferred to the new process and reused? If not, can they 
bind to alternative localhost addresses in the 127.x.y.z space?

Those are all the "easy" options. A crappy option is to pick one of them and 
restart it once as part of startup. Something like:
- Start HMS. Find out what port it's on.
- Start Sentry, configured to talk to HMS's port. Find out what port it's on.
- Restart HMS on the same port, reconfigured to talk to Sentry's port.

Besides inflating the amount of time it takes to start the cluster, HMS's 
ephemeral port may have been snatched up in the restart, so there's potential 
flakiness (or retries, I guess) there.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I03f39cf9b2c813c0c305d085e1ad3851636326f5
Gerrit-Change-Number: 11347
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 29 Aug 2018 21:06:01 +
Gerrit-HasComments: Yes

[kudu-CR] build: fix run-test.sh when not retrying all tests

Andrew Wong has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/11348 )

Change subject: build: fix run-test.sh when not retrying all tests
..

build: fix run-test.sh when not retrying all tests

This is a follow-up to 5d69deb36925113796cd69f51061b8396b0174fc to clean
up some bash syntax. Notably, if KUDU_RETRY_ALL_FAILED_TESTS is unset,
the following message would be logged to stderr:

  line 77: [: : integer expression expected

Here is a dist-test run with KUDU_RETRY_ALL_FAILED_TESTS unset:
http://dist-test.cloudera.org/job?job_id=awong.1535506444.68746#

...and here is one with it set:
http://dist-test.cloudera.org/job?job_id=awong.1535506280.65606

Change-Id: Icb208194fed255361b2987e945ef91779742d4d7
Reviewed-on: http://gerrit.cloudera.org:8080/11348
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M build-support/run-test.sh
1 file changed, 11 insertions(+), 13 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Icb208194fed255361b2987e945ef91779742d4d7
Gerrit-Change-Number: 11348
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] build: fix run-test.sh when not retrying all tests

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11348 )

Change subject: build: fix run-test.sh when not retrying all tests
..


Patch Set 4:

> Patch Set 4: Code-Review+2
>
> If you haven't already, could you manually test the path that runs through 
> that grep statement just to make sure it works properly? If for some reason 
> it always fails, we'll just not retry some tests and may not notice.

Yep, tried on a centos 6.6 running with an artificial list.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb208194fed255361b2987e945ef91779742d4d7
Gerrit-Change-Number: 11348
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 29 Aug 2018 22:22:43 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2560: exported thirdparty library variants should use exported deps

Hello Alexey Serbin, Dan Burkert, Andrew Wong,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: KUDU-2560: exported thirdparty library variants should use 
exported deps
..

KUDU-2560: exported thirdparty library variants should use exported deps

This addresses a longstanding bug in ADD_THIRDPARTY_LIB that caused
"exported" thirdparty library targets to express their own thirdparty
dependencies in terms of regular (i.e. non-exported) targets.

The first time this mattered was in commit d55df3c6e, where libunwind was
added as a dependency of glog. This caused the exported client library, by
virtue of its glog dependency, to depend on 'unwind' (shared object) instead
of on 'unwind_exported' (static archive). In most systems the linker elided
that dependency because it was unnecessary, but it is preserved by the
linker in devtoolset3. That causes client_examples-test to fail if the
system doesn't have libunwind installed.

While I was there I removed some usages of DEPS that were not actually used.

Change-Id: I28dd499b152d4eadd9b5cf6575650a1a3db163f1
---
M CMakeLists.txt
M cmake_modules/FindPmem.cmake
2 files changed, 8 insertions(+), 9 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I28dd499b152d4eadd9b5cf6575650a1a3db163f1
Gerrit-Change-Number: 11354
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert

[kudu-CR] KUDU-2560: exported thirdparty library variants should use exported deps

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11354 )

Change subject: KUDU-2560: exported thirdparty library variants should use 
exported deps
..


Patch Set 1: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I28dd499b152d4eadd9b5cf6575650a1a3db163f1
Gerrit-Change-Number: 11354
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 29 Aug 2018 22:28:20 +
Gerrit-HasComments: No

[kudu-CR] [KUDU-2521] Java Implementation for BloomFilter

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11333 )

Change subject: [KUDU-2521] Java Implementation for BloomFilter
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/11333/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java
File java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java:

http://gerrit.cloudera.org:8080/#/c/11333/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java@31
PS1, Line 31:
> Shall we start the review at this Change-ID ?
Typically you can just rebase and keep pushing to the same change-id / gerrit 
patchset, but in this case we can just continue the review here.


http://gerrit.cloudera.org:8080/#/c/11333/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java@109
PS1, Line 109:   public void put(Long data) {
should this be 'long'?


http://gerrit.cloudera.org:8080/#/c/11333/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java@149
PS1, Line 149:   public static interface HashFunction {
Exposing this abstraction as an enum might be the better way to go, since it's 
a 'closed' set of options.  I think a user could technically implement this 
interface for their custom hash function, which would work until the bloom 
filter was sent to the server, at which point the server would have no idea 
what to do with it.  An enum better models the 'one of a few options, but not 
open to extension' idea.


http://gerrit.cloudera.org:8080/#/c/11333/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java@288
PS1, Line 288:   private static class BloomKeyProbe {
Is the key probe useful?  As I understand it it's used in the server as an 
optimization when looking up whether a key is in multiple bloom filters, but 
since we don't need such optimized lookups in this Java implementation it could 
potentially be removed?  Then you could expose mayContains for specific types 
instead of having to do type dispatch, and the number of methods will be about 
the same.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32673c008f9958088d281c2c198c543bfea2eb8e
Gerrit-Change-Number: 11333
Gerrit-PatchSet: 1
Gerrit-Owner: jinxing6...@126.com
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: jinxing6...@126.com
Gerrit-Comment-Date: Wed, 29 Aug 2018 22:35:33 +
Gerrit-HasComments: Yes

[kudu-CR] [KUDU-2521] Java Implementation for BloomFilter

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11077 )

Change subject: [KUDU-2521] Java Implementation for BloomFilter
..


Patch Set 4:

> Patch Set 4:
>
> (1 comment)

Just left a review, thanks!  Will close this one out in favor of the new 
patchset.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I50c5dce4d24707c068c5ab94e5d311b12b3251b8
Gerrit-Change-Number: 11077
Gerrit-PatchSet: 4
Gerrit-Owner: jinxing6...@126.com
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: jinxing6...@126.com
Gerrit-Comment-Date: Wed, 29 Aug 2018 22:36:19 +
Gerrit-HasComments: No

[kudu-CR] [KUDU-2521] Java Implementation for BloomFilter

Dan Burkert has abandoned this change. ( http://gerrit.cloudera.org:8080/11077 )

Change subject: [KUDU-2521] Java Implementation for BloomFilter
..


Abandoned

Closing in favor of https://gerrit.cloudera.org/c/11333/
--
To view, visit http://gerrit.cloudera.org:8080/11077
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I50c5dce4d24707c068c5ab94e5d311b12b3251b8
Gerrit-Change-Number: 11077
Gerrit-PatchSet: 4
Gerrit-Owner: jinxing6...@126.com
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: jinxing6...@126.com

[kudu-CR](gh-pages) Blogpost describing index skip scan optimization.

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11263 )

Change subject: Blogpost describing index skip scan optimization.
..


Patch Set 4:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md
File _posts/2018-08-17-index-skip-scan-optimization-in-kudu.md:

http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@31
PS4, Line 31: B-Tree
Maybe, add a reference (like https://en.wikipedia.org/wiki/B-tree) in-line or 
in a separate 'References' section?


http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@32
PS4, Line 32: The data is sorted lexicographically starting from the leftmost 
primary key column and stored in the B-Tree leaf nodes.
: Therefore, when the user query contains the first key column 
("host"), Kudu uses the primary key range push down
: operation to optimize the scan time.
> IMO this doesn't convey the idea that the data is sorted by the composite o
+1 all points mentioned by Andrew here.


http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@40
PS4, Line 40: (since the primary key index is sorted on the basis of the first 
key column)
I'm not sure this gives a clear explanation as for the reason to perform a full 
table scan.  Could you update this to explain why simply using the primary 
index we cannot instantly locate the desired rows?


http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@45
PS4, Line 45: The answer is yes
In general, I think the index skip scan optimization is not the only answer.  
In other databases it's possible to build secondary indices, and that might 
work even better (of course it depends on the read/write ratio for the use-case 
and availability of space to build additional index).

I think it's worth mentioning that building secondary index would not be the 
option here since Kudu does not support secondary indices yet.


http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@53
PS4, Line 53: select clusterid from metrics where tstamp = 100
nit: maybe, to be in sync with the CREATE TABLE statement above, write SQL 
keywords in capital letters.


http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@62
PS4, Line 62: popularly known as index skip scan optimization can skip all the 
rows for which host = "helium" and tstamp != 100
> nit: it's great to get to the point that we call this a "skip scan". To dri
Maybe, it's worth mentioning 'skip scan' earlier where you give a short 
overview of the idea behind the skip scan optimization.  Also, as for 
addressing the 'popularity' of the term, I think that adding some references in 
a separate section for various databases that implement that optimization might 
be useful (e.g., one of those links might be 
https://oracle-base.com/articles/9i/index-skip-scanning).


http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@73
PS4, Line 73: Based on experiments on upto 10 million rows per tablet, we 
decided to disable skip scan when the number of seeks
: for distinct prefix column values exceeds 
![](https://latex.codecogs.com/gif.latex?%5Csqrt%7B%5C%23total%20rows%7D).
> This could use some explanation as to why sqrt(total_num_rows) was chosen.
Yep, it would be nice to add some details around the data and reasoning backing 
the choice of this disable-skip-scan criterion.

1) As for those experiments, were those using the table schema and query 
pattern mentioned above?  Or those experiments involved some other table 
schemas and query patterns?
2) What was the rationale at the conceptual level to choose that sqrt() metric?
3) If there were multiple candidate criteria to choose from, maybe it's worth 
mentioning those as well?
4) If 3 is true, was the sqrt() criteria a clear winner or there was some 
fuziness and the sqrt() was chosen also because it looks simpler comparing to 
others?


http://gerrit.cloudera.org:8080/#/c/11263/4/_posts/2018-08-17-index-skip-scan-optimization-in-kudu.md@78
PS4, Line 78: The performance graph of this approach is shown below
This is for the schema and query pattern mentioned earlier, right?  Maybe, it's 
worth mentioning that.



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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: comment
Gerrit-Change-Id: I2250652dcba3d1b0a06f1ffb7f23c11bf533d35e
Gerrit-Change-Number: 11263
Gerrit-PatchSet: 4
Gerrit-Owner: Anupama Gupta 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Comment-Date: Wed, 29 Aug 2018 22:

[kudu-CR] KUDU-2560: exported thirdparty library variants should use exported deps

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11354 )

Change subject: KUDU-2560: exported thirdparty library variants should use 
exported deps
..


Patch Set 1: Code-Review+1

Reading the commit msg, this change makes sense, but I'll defer to Alexey. I 
haven't written enough CMake to know if there are any gotchas to keep in mind.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I28dd499b152d4eadd9b5cf6575650a1a3db163f1
Gerrit-Change-Number: 11354
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 29 Aug 2018 22:48:15 +
Gerrit-HasComments: No

[kudu-CR] [KUDU-2521] Java Implementation for BloomFilter

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11333 )

Change subject: [KUDU-2521] Java Implementation for BloomFilter
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11333/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java
File java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java:

http://gerrit.cloudera.org:8080/#/c/11333/1/java/kudu-client/src/main/java/org/apache/kudu/util/BloomFilter.java@149
PS1, Line 149:   public static interface HashFunction {
> Exposing this abstraction as an enum might be the better way to go, since i
To be clear the implementation probably requires such an interface, but the 
public API is probably better exposed as an enum.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32673c008f9958088d281c2c198c543bfea2eb8e
Gerrit-Change-Number: 11333
Gerrit-PatchSet: 1
Gerrit-Owner: jinxing6...@126.com
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: jinxing6...@126.com
Gerrit-Comment-Date: Wed, 29 Aug 2018 22:50:12 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2560: exported thirdparty library variants should use exported deps

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11354 )

Change subject: KUDU-2560: exported thirdparty library variants should use 
exported deps
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I28dd499b152d4eadd9b5cf6575650a1a3db163f1
Gerrit-Change-Number: 11354
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 30 Aug 2018 01:16:26 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2560: exported thirdparty library variants should use exported deps

Alexey Serbin has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/11354 )

Change subject: KUDU-2560: exported thirdparty library variants should use 
exported deps
..

KUDU-2560: exported thirdparty library variants should use exported deps

This addresses a longstanding bug in ADD_THIRDPARTY_LIB that caused
"exported" thirdparty library targets to express their own thirdparty
dependencies in terms of regular (i.e. non-exported) targets.

The first time this mattered was in commit d55df3c6e, where libunwind was
added as a dependency of glog. This caused the exported client library, by
virtue of its glog dependency, to depend on 'unwind' (shared object) instead
of on 'unwind_exported' (static archive). In most systems the linker elided
that dependency because it was unnecessary, but it is preserved by the
linker in devtoolset3. That causes client_examples-test to fail if the
system doesn't have libunwind installed.

While I was there I removed some usages of DEPS that were not actually used.

Change-Id: I28dd499b152d4eadd9b5cf6575650a1a3db163f1
Reviewed-on: http://gerrit.cloudera.org:8080/11354
Reviewed-by: Dan Burkert 
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong 
Reviewed-by: Alexey Serbin 
---
M CMakeLists.txt
M cmake_modules/FindPmem.cmake
2 files changed, 8 insertions(+), 9 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, but someone else must approve
  Kudu Jenkins: Verified
  Andrew Wong: Looks good to me, but someone else must approve
  Alexey Serbin: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I28dd499b152d4eadd9b5cf6575650a1a3db163f1
Gerrit-Change-Number: 11354
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2560: exported thirdparty library variants should use exported deps

2018-08-29 Thread Grant Henke (Code Review)

Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11354 )

Change subject: KUDU-2560: exported thirdparty library variants should use 
exported deps
..


Patch Set 2:

Thanks for solving this!


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I28dd499b152d4eadd9b5cf6575650a1a3db163f1
Gerrit-Change-Number: 11354
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 30 Aug 2018 01:40:35 +
Gerrit-HasComments: No

[kudu-CR] bitmap: add equality method

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11266 )

Change subject: bitmap: add equality method
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11266/4/src/kudu/util/bitmap-test.cc
File src/kudu/util/bitmap-test.cc:

http://gerrit.cloudera.org:8080/#/c/11266/4/src/kudu/util/bitmap-test.cc@231
PS4, Line 231: TEST(TestBitMap, TestEquals) {
Does it make sense to add a test for overlapping bitmaps as well?


http://gerrit.cloudera.org:8080/#/c/11266/4/src/kudu/util/bitmap-test.cc@254
PS4, Line 254: i
Should this be (i + 1)?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If53e41250435501e158bdbc076dc8e89ea153256
Gerrit-Change-Number: 11266
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 30 Aug 2018 01:50:12 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2469 pt 2: fail replicas on CFile corruption

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke,

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

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

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

Change subject: KUDU-2469 pt 2: fail replicas on CFile corruption
..

KUDU-2469 pt 2: fail replicas on CFile corruption

This adds handling for CFile corruption errors via the error manager. If
a CFile corruption is encountered, the tablet affected will be failed
and scheduled to be shutdown, pulling the tablet id of interest from the
IOContext.

Corruption handling is delegated to the CFileReaders, which have access
to the error manager. If a CFileReader method is to return a corruption
error, it first must call the appropriate error-handling. Wrappers
classes of CFileReaders that yield corruption errors can do the same.

This patch includes some fault injection macros that helped facilitate
testing, and some extra plumbing of IOContexts in places that were
caught without coverage: the IndexTreeIterator, the BloomCache, and
DeltaFileReader::ReadDeltaStats().

Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/cfile/index_btree.cc
M src/kudu/cfile/index_btree.h
M src/kudu/fs/error_manager.cc
M src/kudu/fs/error_manager.h
M src/kudu/fs/io_context.h
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_server.cc
15 files changed, 385 insertions(+), 99 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f
Gerrit-Change-Number: 11249
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2469 pt 2: fail replicas on CFile corruption

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11249 )

Change subject: KUDU-2469 pt 2: fail replicas on CFile corruption
..


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11249/6/src/kudu/cfile/index_btree.h
File src/kudu/cfile/index_btree.h:

http://gerrit.cloudera.org:8080/#/c/11249/6/src/kudu/cfile/index_btree.h@110
PS6, Line 110:   Status LoadBlock(const BlockPointer &block, int depth);
> warning: function 'kudu::cfile::IndexTreeIterator::LoadBlock' has a definit
Done


http://gerrit.cloudera.org:8080/#/c/11249/6/src/kudu/cfile/index_btree.cc
File src/kudu/cfile/index_btree.cc:

http://gerrit.cloudera.org:8080/#/c/11249/6/src/kudu/cfile/index_btree.cc@318
PS6, Line 318: }
> warning: do not use 'else' after 'return' [readability-else-after-return]
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f
Gerrit-Change-Number: 11249
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 30 Aug 2018 04:09:03 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2469 pt 2: fail replicas on CFile corruption

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke,

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

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

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

Change subject: KUDU-2469 pt 2: fail replicas on CFile corruption
..

KUDU-2469 pt 2: fail replicas on CFile corruption

This adds handling for CFile corruption errors via the error manager. If
a CFile corruption is encountered, the tablet affected will be failed
and scheduled to be shutdown, pulling the tablet id of interest from the
IOContext.

Corruption handling is delegated to the CFileReaders, which have access
to the error manager. If a CFileReader method is to return a corruption
error, it first must call the appropriate error-handling. Wrappers
classes of CFileReaders that yield corruption errors can do the same.

This patch includes some fault injection macros that helped facilitate
testing, and some extra plumbing of IOContexts in places that were
caught without coverage: the IndexTreeIterator, the BloomCache, and
DeltaFileReader::ReadDeltaStats().

Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/cfile/index_btree.cc
M src/kudu/cfile/index_btree.h
M src/kudu/fs/error_manager.cc
M src/kudu/fs/error_manager.h
M src/kudu/fs/io_context.h
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_server.cc
15 files changed, 392 insertions(+), 104 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/11249/8
--
To view, visit http://gerrit.cloudera.org:8080/11249
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f
Gerrit-Change-Number: 11249
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2469 pt 2: fail replicas on CFile corruption

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11249 )

Change subject: KUDU-2469 pt 2: fail replicas on CFile corruption
..


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11249/7/src/kudu/cfile/cfile_reader.h
File src/kudu/cfile/cfile_reader.h:

http://gerrit.cloudera.org:8080/#/c/11249/7/src/kudu/cfile/cfile_reader.h@115
PS7, Line 115:   // Reads the data block pointed to by `ptr`. Will pull the 
data block from
> warning: missing username/bug in TODO [google-readability-todo]
Rewrote this comment


http://gerrit.cloudera.org:8080/#/c/11249/7/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:

http://gerrit.cloudera.org:8080/#/c/11249/7/src/kudu/cfile/cfile_reader.cc@59
PS7, Line 59: #include "kudu/util/debug/trace_event.h"
> warning: #includes are not sorted properly [llvm-include-order]
Removed this



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f
Gerrit-Change-Number: 11249
Gerrit-PatchSet: 8
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 30 Aug 2018 05:29:19 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2469 pt 2: fail replicas on CFile corruption

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Grant Henke,

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

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

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

Change subject: KUDU-2469 pt 2: fail replicas on CFile corruption
..

KUDU-2469 pt 2: fail replicas on CFile corruption

This adds handling for CFile corruption errors via the error manager. If
a CFile corruption is encountered, the tablet affected will be failed
and scheduled to be shutdown, pulling the tablet id of interest from the
IOContext.

Corruption handling is delegated to the CFileReaders, which have access
to the error manager. If a CFileReader method is to return a corruption
error, it first must call the appropriate error-handling. Wrappers
classes of CFileReaders that yield corruption errors can do the same.

This patch includes some fault injection macros that helped facilitate
testing, and some extra plumbing of IOContexts in places that were
caught without coverage: the IndexTreeIterator, the BloomCache, and
DeltaFileReader::ReadDeltaStats().

Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/cfile/index_btree.cc
M src/kudu/cfile/index_btree.h
M src/kudu/fs/error_manager.cc
M src/kudu/fs/error_manager.h
M src/kudu/fs/io_context.h
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_server.cc
15 files changed, 399 insertions(+), 105 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/11249/9
--
To view, visit http://gerrit.cloudera.org:8080/11249
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f
Gerrit-Change-Number: 11249
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2469 pt 2: fail replicas on CFile corruption

Andrew Wong has removed a vote on this change.

Change subject: KUDU-2469 pt 2: fail replicas on CFile corruption
..


Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11249
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I63d541443bc68c83fd0ca6d51315143fee04d50f
Gerrit-Change-Number: 11249
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2469 pt 2: fail replicas on CFile corruption