[kudu-CR] WIP: [iwyu] first pass

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: [iwyu] first pass
..

WIP: [iwyu] first pass

Updated C++ source files in accordance with include-what-you-use
recommendations:
  * remove unused header files
  * add missing header files
  * use forward declarations when possible

As a result, time of compilation reduced ~10% if building with GNU make
in parallel with 48 jobs (make -j48) at ve0518.halxg.cloudera.com:

prior:
  real  1m21.858s
  user  33m26.685s
  sys 2m19.831s

after:
  real  1m12.686s
  user  30m26.891s
  sys 2m7.627s

The next step is automating the checks, so the IWYU check would run
automatically (like the LINT build).

Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
---
M src/kudu/benchmarks/rle.cc
M src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc
M src/kudu/benchmarks/tpch/rpc_line_item_dao.cc
M src/kudu/benchmarks/tpch/rpc_line_item_dao.h
M src/kudu/benchmarks/tpch/tpch-schemas.h
M src/kudu/benchmarks/tpch/tpch1.cc
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/benchmarks/wal_hiccup.cc
M src/kudu/cfile/binary_dict_block.cc
M src/kudu/cfile/binary_dict_block.h
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/cfile/binary_plain_block.h
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/binary_prefix_block.h
M src/kudu/cfile/block_cache-test.cc
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/block_cache.h
M src/kudu/cfile/block_compression.cc
M src/kudu/cfile/block_compression.h
M src/kudu/cfile/bloomfile-test-base.h
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test-base.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/cfile_util.cc
M src/kudu/cfile/cfile_util.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/cfile/compression-test.cc
M src/kudu/cfile/compression_codec.cc
M src/kudu/cfile/compression_codec.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/gvint_block.cc
M src/kudu/cfile/gvint_block.h
M src/kudu/cfile/index-test.cc
M src/kudu/cfile/index_block.cc
M src/kudu/cfile/index_block.h
M src/kudu/cfile/index_btree.cc
M src/kudu/cfile/index_btree.h
M src/kudu/cfile/mt-bloomfile-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/cfile/type_encodings.h
M src/kudu/client/batcher.cc
M src/kudu/client/batcher.h
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client_builder-internal.h
M src/kudu/client/error-internal.cc
M src/kudu/client/error-internal.h
M src/kudu/client/error_collector.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/replica-internal.cc
M src/kudu/client/replica-internal.h
M src/kudu/client/resource_metrics.cc
M src/kudu/client/resource_metrics.h
M src/kudu/client/samples/sample.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/client/shared_ptr.h
M src/kudu/client/table-internal.cc
M src/kudu/client/table-internal.h
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/table_alterer-internal.h
M src/kudu/client/table_creator-internal.cc
M src/kudu/client/table_creator-internal.h
M src/kudu/client/tablet-internal.h
M src/kudu/client/tablet_server-internal.cc
M src/kudu/client/tablet_server-internal.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/client/write_op.cc
M src/kudu/client/write_op.h
M src/kudu/codegen/code_cache.cc
M src/kudu/codegen/code_cache.h
M src/kudu/codegen/code_generator.cc
M src/kudu/codegen/code_generator.h
M src/kudu/codegen/codegen-test.cc
M src/kudu/codegen/compilation_manager.cc
M src/kudu/codegen/compilation_manager.h
M src/kudu/codegen/jit_wrapper.cc
M src/kudu/codegen/jit_wrapper.h
M src/kudu/codegen/module_builder.cc
M src/kudu/codegen/module_builder.h
M src/kudu/codegen/precompiled.cc
M src/kudu/codegen/row_projector.cc
M src/kudu/codegen/row_projector.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc

[kudu-CR] KUDU-1757: fix appendCellValueDebugString, do not throw exception when a column is not set

[YanlongZheng (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1757: fix appendCellValueDebugString, do not throw 
exception when a column is not set
..

KUDU-1757: fix appendCellValueDebugString, do not throw exception when a column 
is not set

Change-Id: Ife05af5a11e95751b3cc259e786d36494aeb5d3a
---
M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestOperation.java
2 files changed, 9 insertions(+), 7 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife05af5a11e95751b3cc259e786d36494aeb5d3a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: YanlongZheng 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: WIP: KUDU-1767. Create a client flush interleave test
..


Patch Set 2:

Thanks for the review, Alexey. Yes, this test is currently pretty messy. I do 
need to clean it up but I think we need to come up with a real solution to the 
problem it exposes before I need to do that. :)

Based on comments on KUDU-1767 I think we'll be prioritizing some other things 
sooner.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-798 (part 1) Unify leader/follower mvcc behavior

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-798 (part 1) Unify leader/follower mvcc behavior
..


Patch Set 12: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3ba7212f9211f585d4bef00e5ccfc24d5eece224
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] encodings: change IsBlockFull() to not take a limit parameter

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: encodings: change IsBlockFull() to not take a limit parameter
..


Patch Set 3: Code-Review+2

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

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

[kudu-CR] bshuf block: some code cleanup

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: bshuf_block: some code cleanup
..


Patch Set 6: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
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] bshuf block: some code cleanup

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: bshuf_block: some code cleanup
..


Patch Set 6:

(1 comment)

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

Line 22: #include "kudu/gutil/casts.h"
Don't think this is actually used?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
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: Yes

[kudu-CR] bshuf block: some code cleanup

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: bshuf_block: some code cleanup
..


Patch Set 7: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
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] bshuf block: some code cleanup

[Todd Lipcon (Code Review)]

Hello Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: bshuf_block: some code cleanup
..

bshuf_block: some code cleanup

* Some typo/grammar fixes/reformatting in comments.
* Rename kMaxHeaderSize to kHeaderSize since the header is fixed-length.
* Adds a private cell_ptr() function instead of error prone indexing
  with multiplication into the data_ buffer.
* Remove an unnecessarily UINT32 specialization
* Fix some C-style casts and unnecessary type punning in favor of
  memcpy()
* Cleaned up padding code to use KUDU_ALIGN_UP

This also adds a TODO for a bug with GetLastKey: we are mistakenly
indexing into data_[count - 1] instead of data_[(count - 1) *
size_of_type] which causes incorrect results. The fix for this is in the
following commit.

Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
2 files changed, 88 insertions(+), 79 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
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 

[kudu-CR] bshuf block: some code cleanup

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: bshuf_block: some code cleanup
..


Patch Set 6:

(1 comment)

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

Line 22: #include "kudu/gutil/casts.h"
> Don't think this is actually used?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
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: Yes

[kudu-CR] KUDU-1766: Java client partition pruning with MAX VALUE equality predicate fails

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1766: Java client partition pruning with MAX_VALUE 
equality predicate fails
..


Patch Set 1:

(1 comment)

Just passing through...

http://gerrit.cloudera.org:8080/#/c/5266/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java
File 
java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java:

Line 294: // c < MIN
Do we have this new coverage in the C++ tests already?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iad6d00d9f401c510e14709323e7686bfa6d1b449
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] bshuf block: fix GetFirstKey() and GetLastKey()

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: bshuf_block: fix GetFirstKey() and GetLastKey()
..


Patch Set 6: Code-Review+2

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

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

[kudu-CR] Add snapshot scans to fuzz-itest

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add snapshot scans to fuzz-itest
..


Patch Set 12:

(15 comments)

http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:

PS12, Line 292: found
> nit: consider renaming it into something more tied to the scope of this met
Done


PS12, Line 298: for (auto& found_row : found) {
  :   errors->push_back(Substitute("Found unexpected row: 
$0", found_row.ToString()));
  : }
> nit: there is a piece of syntactic sugar for this, if you wish:
I like the incantation but I think its less readable. also strictly its more 
chars so not so sugary :)


PS12, Line 303: (*iter).
> nit: using 'iter->first' instead could save 2 characters and easier to read
Done


PS12, Line 304: vector
> nit: could 'auto' work here as well?
Done


PS12, Line 319:  
> nit: misaligned indent
Done


PS12, Line 324:  
> nit: misaligned indent
Done


PS12, Line 331:  
> nit: misaligned indent
Done


PS12, Line 574: down_cast(
  : tablet()->clock().get())
> nit: on the note of failing gracefully in case of unexpected things in the 
we shouldn't use dynamic_cast on non-debug builds


PS12, Line 577: If it is
> nit: update a bit for better readability
Done


http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/tablet/key_value_test_schema.h
File src/kudu/tablet/key_value_test_schema.h:

PS12, Line 24: #include 
> nit: is it really needed after the change?
Done


PS12, Line 47: string ret = strings::Substitute("{$0,", key);
 : if (val == boost::none) {
 :   ret.append("NULL}");
 : } else {
 :   ret.append(strings::Substitute("$0}", *val));
 : }
 : return ret;
> nit: consider replacing with:
Done


http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

PS12, Line 1754: tmp_snap_timestamp.FromUint64(scan_pb.snap_timestamp());
> nit: consider moving this after the if (!s.ok() && ...) check.
Done


PS12, Line 1757: !s.ok() && 
PREDICT_TRUE(!FLAGS_scanner_allow_snapshot_scans_with_logical_timestamps
> nit: does it make sense to check for the type of the error returned before 
good point. Done


PS12, Line 1758: Snapshot scans not supported on this server
> nit: does it make sense to update the error message to give a hint on the n
I don't think so, honestly, this should only happen for LogicalClocks which are 
a test only thing


PS12, Line 1761: tmp_snap_timestamp > max_allowed_ts
> I might miss something here, but it seems it's always true if GetGlobalLate
if not set by GlobalLatest max_allowed_ts is kInvalidTimestmap, which in turn 
is max int64 -1


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files
..

KUDU-237 (part 2) - Add support for REINSERT in delta files

This patch goes the final mile in adding support for REINSERTs in
delta files. It does various things to achieve this:

- Transforms REDO DELETE/REINSERT mutation pairs into UNDO
REINSERT/DELETE mutation pairs, leaving at the most a REDO
delete.

- Merges ghost rows on compaction: When duplicated rows are
found a new algorithm finds out which one is the most recent
and adds the other one as a 'previous_ghost'. This can happen
for an arbitrary number of ghost rows. Noteworthy is that setting
previous versions requires a copy. The two rows are in different
RowBlocks (for row data) and Arenas (for mutations) and it is
not guaranteed that the previous ghost will suvive by the time the
row that points to it is processed.

- Adds new test to tablet-test and changes a test in
compaction-test proving that this works.

- Adds a new test to compaction-test that creates several layers
of overlapping rowsets where each layer has one rowset less than
the previous one. This creates a mix of duplicated (up to 10
different ghosts) and unique rows. The test then compacts the
rowsets a few at a time and makes sure the histories are correct.

Follow up patches will add new itests that test this functionality
even more broadly.

Ran 500 loops of mt-tablet-test and compaction-test
in dist-test with "KUDU_ALLOW_SLOW_TESTS=1" and
"--stress_cpu_threads=4". All tests passed. Results:

compaction-test: 
http://dist-test.cloudera.org//job?job_id=david.alves.1480028902.31041
mt-tablet-test:  
http://dist-test.cloudera.org//job?job_id=david.alves.1480028908.31125

Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
---
M src/kudu/common/row_changelist.h
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/tablet-test.cc
8 files changed, 734 insertions(+), 193 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/4995/22
-- 
To view, visit http://gerrit.cloudera.org:8080/4995
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
Gerrit-PatchSet: 22
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:

Line 90:   row_block_.reset(new RowBlock(iter_->schema(), num_in_block, 
_));
> well, it avoids an extra bit of copying, right?
removed


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] encodings: change IsBlockFull() to not take a limit parameter

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: encodings: change IsBlockFull() to not take a limit parameter
..


encodings: change IsBlockFull() to not take a limit parameter

Everywhere we called this, we used the same value (the configured cfile
block size). Given that, it's simpler to not take the parameter. This
makes it easier for an encoder to determine whether or not it is full in
some other context, since it can safely assume that it knows the block
size up front.

Change-Id: I4598c1555732649eeebf43231082dd641ecfc576
Reviewed-on: http://gerrit.cloudera.org:8080/5200
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/cfile/binary_dict_block.cc
M src/kudu/cfile/binary_dict_block.h
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/cfile/binary_plain_block.h
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/binary_prefix_block.h
M src/kudu/cfile/block_encodings.h
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/gvint_block.cc
M src/kudu/cfile/gvint_block.h
M src/kudu/cfile/plain_bitmap_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/cfile/type_encodings.cc
16 files changed, 60 insertions(+), 48 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I4598c1555732649eeebf43231082dd641ecfc576
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP: Support safe time advancement on replicas in the absense of writes

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: WIP: Support safe time advancement on replicas in the absense 
of writes
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5240/3/src/kudu/consensus/time_manager.cc
File src/kudu/consensus/time_manager.cc:

Line 211: } // consensus
> warning: namespace 'consensus' ends with an unrecognized comment [google-re
Done


Line 212: } // kudu
> warning: namespace 'kudu' ends with an unrecognized comment [google-readabi
Done


http://gerrit.cloudera.org:8080/#/c/5240/3/src/kudu/consensus/time_manager.h
File src/kudu/consensus/time_manager.h:

Line 34:   TimeManager(const scoped_refptr& clock);
> warning: single-argument constructors must be marked explicit to avoid unin
Done


http://gerrit.cloudera.org:8080/#/c/5240/3/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:

Line 240:   Status WaitForSnapshotWithAllCommitted(Timestamp timestamp,
> warning: function 'kudu::tablet::MvccManager::WaitForSnapshotWithAllCommitt
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8532fdb069c8bee7f3e08ffe74cab0273885cc8e
Gerrit-PatchSet: 3
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] [i-tests] test for timestamp propagation with write ops

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: [i-tests] test for timestamp propagation with write ops
..

[i-tests] test for timestamp propagation with write ops

Added an integration test to verify that C++ client propagates
timestamp with write operations.

The test is disabled as it currently fails. A follow up patch will fix
the bug and enable the test.

This is in the context of the following JIRA item:
  https://issues.apache.org/jira/browse/KUDU-1770

Change-Id: I1be285fd860f0608f5d4ce3be57c4e4b04c63455
---
M src/kudu/integration-tests/consistency-itest.cc
1 file changed, 55 insertions(+), 0 deletions(-)


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

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

[kudu-CR] bshuf block: some low-hanging-fruit optimizations on write path

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: bshuf_block: some low-hanging-fruit optimizations on write path
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5195/5//COMMIT_MSG
Commit Message:

Line 9: Rather than adding an element at a time and calling the virtual
gvint_block.cc is doing something very similar.  Perhaps it would be good to 
keep the implementation in-sync WRT this behavior?


http://gerrit.cloudera.org:8080/#/c/5195/5/src/kudu/cfile/bshuf_block.h
File src/kudu/cfile/bshuf_block.h:

Line 165:   size_t EstimateEncodedSize() const {
This is no longer used.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia895f7731e5371967782ef9cb176a9d493894a83
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files
..


Patch Set 22: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
Gerrit-PatchSet: 22
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [i-tests] scan token timestamp propagation test

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:

PS1, Line 611: ASSERT_OK(KuduScanToken::DeserializeIntoScanner(client.get(), 
scan_token,
 : 
_raw));
how about here we assert that the lot here is 'ts' from the block above and at 
that the lot after each scan is higher?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
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: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] Don't output unobservable rows from the MemRowset

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Don't output unobservable rows from the MemRowset
..

Don't output unobservable rows from the MemRowset

In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.

The sequence is as follows:

Insert Row at 1 -> Row goes in the MRS
Flush   -> Row now lives in RS1
Delete Row at 3 -> Row is now a ghost in RS1
Insert Row at 3 -> Row goes in the MRS again
Delete Row at 3 -> Row is deleted from the MRS
Flush   -> Row is now a ghost in RS2
Major delta compact RS1 -> GC all before 3
Major delta compact RS2 -> GC all before 3

If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.

The point is that we can't trust UNDOs to break ties, regarding
recency, between two ghosts as they might have been removed by
delta compactions. However we can always trust REDO delete/reinsert
to remain.

This adds a small test to compaction-test that makes sure that
a row that is inserted and deleted in the same transaction doesn't
appear in the compaction input.

Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 186 insertions(+), 62 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/15
-- 
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files
..

KUDU-237 (part 2) - Add support for REINSERT in delta files

This patch goes the final mile in adding support for REINSERTs in
delta files. It does various things to achieve this:

- Transforms REDO DELETE/REINSERT mutation pairs into UNDO
REINSERT/DELETE mutation pairs, leaving at the most a REDO
delete.

- Merges ghost rows on compaction: When duplicated rows are
found a new algorithm finds out which one is the most recent
and adds the other one as a 'previous_ghost'. This can happen
for an arbitrary number of ghost rows. Noteworthy is that setting
previous versions requires a copy. The two rows are in different
RowBlocks (for row data) and Arenas (for mutations) and it is
not guaranteed that the previous ghost will suvive by the time the
row that points to it is processed.

- Adds new test to tablet-test and changes a test in
compaction-test proving that this works.

- Adds a new test to compaction-test that creates several layers
of overlapping rowsets where each layer has one rowset less than
the previous one. This creates a mix of duplicated (up to 10
different ghosts) and unique rows. The test then compacts the
rowsets a few at a time and makes sure the histories are correct.

Follow up patches will add new itests that test this functionality
even more broadly.

Ran 500 loops of mt-tablet-test and compaction-test
in dist-test with "KUDU_ALLOW_SLOW_TESTS=1" and
"--stress_cpu_threads=4". All tests passed. Results:

compaction-test: 
http://dist-test.cloudera.org//job?job_id=david.alves.1480028902.31041
mt-tablet-test:  
http://dist-test.cloudera.org//job?job_id=david.alves.1480028908.31125

Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
---
M src/kudu/common/row_changelist.h
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/tablet-test.cc
8 files changed, 736 insertions(+), 194 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/4995/20
-- 
To view, visit http://gerrit.cloudera.org:8080/4995
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
Gerrit-PatchSet: 20
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files
..


Patch Set 20:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4995/20/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:

PS20, Line 625: int
this should be a sized int type, no?


Line 664:   // Expecte an UNDO reinsert for the delete.
typo


http://gerrit.cloudera.org:8080/#/c/4995/12/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:

Line 90:   row_block_.reset(new RowBlock(iter_->schema(), num_in_block, 
_));
> I guess not really since we don't do flushing compactions (where we would n
well, it avoids an extra bit of copying, right?


http://gerrit.cloudera.org:8080/#/c/4995/20/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:

PS20, Line 233: 0;
nit: should be a '.'


PS20, Line 658: OrNull(
I think 'OrNull' is confusing here, because you're not advancing "while it's 
null". I think you can just remove it, since it's kind of implicit that we 
won't advance past the end of the linked list.


PS20, Line 669:head = right;
  : return head;
why not just 'return right;' (same below)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
Gerrit-PatchSet: 20
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to 
".kudutmp"
..


Patch Set 3:

Hey Maxim, looks like this patch has some conflicts and needs to be rebased.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add snapshot scans to fuzz-itest

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add snapshot scans to fuzz-itest
..

Add snapshot scans to fuzz-itest

This adds a new operation to fuzz-itest: snapshot scans
at a timestamp.

When generating random operations, scans at a timestamp are
now in the mix. The generator records how timestamps will
progress with writes and is careful to make sure that scans
happen in the past. After the test case is generated, but
before it is run, the timestamps for the scans are deduplicated
and sorted to that we save the state immediately before those
(and only those) timestamps.

This would fail very often before the REINSERT patches but
passes with it.

Ran 500 loops of fuzz-itest in dist-test in ASAN with:
- KUDU_ALLOW_SLOW_TESTS=1
- --stress_cpu_threads=4

All tests passed. Result:
http://dist-test.cloudera.org//job?job_id=david.alves.1480096588.30319

Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
---
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/tablet/key_value_test_schema.h
M src/kudu/tserver/tablet_service.cc
3 files changed, 206 insertions(+), 48 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/4996/14
-- 
To view, visit http://gerrit.cloudera.org:8080/4996
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files
..

KUDU-237 (part 2) - Add support for REINSERT in delta files

This patch goes the final mile in adding support for REINSERTs in
delta files. It does various things to achieve this:

- Transforms REDO DELETE/REINSERT mutation pairs into UNDO
REINSERT/DELETE mutation pairs, leaving at the most a REDO
delete.

- Merges ghost rows on compaction: When duplicated rows are
found a new algorithm finds out which one is the most recent
and adds the other one as a 'previous_ghost'. This can happen
for an arbitrary number of ghost rows. Noteworthy is that setting
previous versions requires a copy. The two rows are in different
RowBlocks (for row data) and Arenas (for mutations) and it is
not guaranteed that the previous ghost will suvive by the time the
row that points to it is processed.

- Adds new test to tablet-test and changes a test in
compaction-test proving that this works.

- Adds a new test to compaction-test that creates several layers
of overlapping rowsets where each layer has one rowset less than
the previous one. This creates a mix of duplicated (up to 10
different ghosts) and unique rows. The test then compacts the
rowsets a few at a time and makes sure the histories are correct.

Follow up patches will add new itests that test this functionality
even more broadly.

Ran 500 loops of mt-tablet-test and compaction-test
in dist-test with "KUDU_ALLOW_SLOW_TESTS=1" and
"--stress_cpu_threads=4". All tests passed. Results:

compaction-test: 
http://dist-test.cloudera.org//job?job_id=david.alves.1480028902.31041
mt-tablet-test:  
http://dist-test.cloudera.org//job?job_id=david.alves.1480028908.31125

Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
---
M src/kudu/common/row_changelist.h
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/tablet-test.cc
8 files changed, 735 insertions(+), 194 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/4995/21
-- 
To view, visit http://gerrit.cloudera.org:8080/4995
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
Gerrit-PatchSet: 21
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files
..


Patch Set 20:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4995/20/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:

PS20, Line 625: int
> this should be a sized int type, no?
Done


Line 664:   // Expecte an UNDO reinsert for the delete.
> typo
Done


http://gerrit.cloudera.org:8080/#/c/4995/20/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:

PS20, Line 233: 0;
> nit: should be a '.'
Done


PS20, Line 658: OrNull(
> I think 'OrNull' is confusing here, because you're not advancing "while it'
Done


PS20, Line 669:head = right;
  : return head;
> why not just 'return right;' (same below)
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
Gerrit-PatchSet: 20
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1766: Java client partition pruning with MAX VALUE equality predicate fails

[Dan Burkert (Code Review)]

Hello Adar Dembo, Todd Lipcon,

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

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

to review the following change.

Change subject: KUDU-1766: Java client partition pruning with MAX_VALUE 
equality predicate fails
..

KUDU-1766: Java client partition pruning with MAX_VALUE equality predicate fails

The bug orginated in a mistransliteration of the c++ partition pruning
algorithm; the original version has always had the missing if statement:
https://github.com/apache/kudu/blob/53e67e9eb7b4fc940a14a17119041871e80bcc3f/src/kudu/common/key_util.cc#L152-L155

Change-Id: Iad6d00d9f401c510e14709323e7686bfa6d1b449
---
M java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java
2 files changed, 26 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iad6d00d9f401c510e14709323e7686bfa6d1b449
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1766: Java client partition pruning with MAX VALUE equality predicate fails

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1766: Java client partition pruning with MAX_VALUE 
equality predicate fails
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iad6d00d9f401c510e14709323e7686bfa6d1b449
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP: Support safe time advancement on replicas in the absense of writes

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: Support safe time advancement on replicas in the absense 
of writes
..

WIP: Support safe time advancement on replicas in the absense of writes

Mostly getting some jenkins mileage but the design should be close
to the final one.

This is a WIP that needs to be broken down further but that summarily
adds the following:

- Add a TimeManager class: This class is responsible for tracking
  safe time advancement on leaders and replicas.
- Removes the clock from MvccManager - Since safe time is now managed
  by the TimeManager there is no need for Mvcc to have one internally.
- Changes how we wait for "all committed" in MvccManager. Previously
  we would wait for a clean snapshot. Now we instead wait for all
  transactions before the timestamp to be committed even if the
  clean timestamp doesn't advance. This is because without leader
  leases the safe time can go back, but moving the clean time back
  would break a lot of things internally.
- Plugs the TimeManager in consensus/queue and calls its methods
  appropriately.

Change-Id: I8532fdb069c8bee7f3e08ffe74cab0273885cc8e
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus.h
M src/kudu/consensus/consensus.proto
M src/kudu/consensus/consensus_peers-test.cc
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
A src/kudu/consensus/time_manager-test.cc
A src/kudu/consensus/time_manager.cc
A src/kudu/consensus/time_manager.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_peer.cc
M src/kudu/tablet/tablet_peer.h
M src/kudu/tablet/transactions/alter_schema_transaction.cc
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
31 files changed, 584 insertions(+), 174 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8532fdb069c8bee7f3e08ffe74cab0273885cc8e
Gerrit-PatchSet: 4
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 

[kudu-CR] bshuf block: fix GetFirstKey() and GetLastKey()

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: bshuf_block: fix GetFirstKey() and GetLastKey()
..


bshuf_block: fix GetFirstKey() and GetLastKey()

This fixes a bug where the bitshuffle encoding would return the wrong
value for GetFirstKey() and GetLastKey(). The issue was that, in the
case of UINT32s, the data_ array was being inspected after 'Finish()'
had already collapsed down the elements to their minimum width.

Since this collapsing is only done on UINT32 types, this would only
affect these functions on dictionary-encoded binary blocks, which use
bitshuffle internally for storing the UINT32 code words. However, the
"cleanup" commit prior to this one also fixed some incorrect indexing
that would affect BIT_SHUFFLE blocks outside the context of UINT32.

These functions are only used in the case that the column in question is
a non-composite primary key, in order to build the value index as well
as to determine the min/max bounds of a written CFile.

The result of returning the wrong value for GetLastKey() was that the
resulting index blocks would be incorrect, and seeks based on key would
end up at the wrong row. This could cause spurious "row not found"
errors or even crashes in some cases.

Apparently real users have not made use of BITSHUFFLE or DICT_ENCODING
on such columns, since we haven't seen reports of this in the wild; I
instead discovered this issue while working on KUDU-1751 (changing those
encodings to be the default).

The patch fixes the issue and also expands test coverage to cover the
GetFirstKey/GetLastKey calls.

Change-Id: I83dbc01ebf7a10e69b7fff6b7973967ebf6b4580
Reviewed-on: http://gerrit.cloudera.org:8080/5192
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/encoding-test.cc
3 files changed, 49 insertions(+), 11 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I83dbc01ebf7a10e69b7fff6b7973967ebf6b4580
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] cfile-test: use a faster data generator for 100M-string test

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: cfile-test: use a faster data generator for 100M-string test
..


Patch Set 2: Code-Review+2

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

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

[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files

[David Ribeiro Alves (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files
..

KUDU-237 (part 2) - Add support for REINSERT in delta files

This patch goes the final mile in adding support for REINSERTs in
delta files. It does various things to achieve this:

- Transforms REDO DELETE/REINSERT mutation pairs into UNDO
REINSERT/DELETE mutation pairs, leaving at the most a REDO
delete.

- Merges ghost rows on compaction: When duplicated rows are
found a new algorithm finds out which one is the most recent
and adds the other one as a 'previous_ghost'. This can happen
for an arbitrary number of ghost rows. Noteworthy is that setting
previous versions requires a copy. The two rows are in different
RowBlocks (for row data) and Arenas (for mutations) and it is
not guaranteed that the previous ghost will suvive by the time the
row that points to it is processed.

- Adds new test to tablet-test and changes a test in
compaction-test proving that this works.

- Adds a new test to compaction-test that creates several layers
of overlapping rowsets where each layer has one rowset less than
the previous one. This creates a mix of duplicated (up to 10
different ghosts) and unique rows. The test then compacts the
rowsets a few at a time and makes sure the histories are correct.

Follow up patches will add new itests that test this functionality
even more broadly.

Ran 500 loops of mt-tablet-test and compaction-test
in dist-test with "KUDU_ALLOW_SLOW_TESTS=1" and
"--stress_cpu_threads=4". All tests passed. Results:

compaction-test: 
http://dist-test.cloudera.org//job?job_id=david.alves.1480028902.31041
mt-tablet-test:  
http://dist-test.cloudera.org//job?job_id=david.alves.1480028908.31125

Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
---
M src/kudu/common/row_changelist.h
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/tablet-test.cc
8 files changed, 634 insertions(+), 134 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/4995/23
-- 
To view, visit http://gerrit.cloudera.org:8080/4995
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
Gerrit-PatchSet: 23
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] bshuf block: some code cleanup

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: bshuf_block: some code cleanup
..


Patch Set 6:

(1 comment)

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

PS6, Line 152:   uint32_t value = array[i];
 :   memcpy([i * sizeof(uint32_t)], , 
sizeof(value));
nit: I missed that while doing the review, but memmove() might be a little bit 
faster:

  https://gist.github.com/alexeyserbin/9a1612f13c22d4df1946c8831a8b7910


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
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: Yes

[kudu-CR] bshuf block: some code cleanup

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: bshuf_block: some code cleanup
..


Patch Set 6:

(1 comment)

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

PS6, Line 152:   uint32_t value = array[i];
 :   memcpy([i * sizeof(uint32_t)], , 
sizeof(value));
> I don't get the same results you did, and also typically memcpy is faster t
Oops, my bad: the snipped was lacking the actual looping.  With that fixed, the 
results are closer to the expected ones: I updated the gist.

Sorry for the noise.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
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: Yes

[kudu-CR] KUDU-1766: Java client partition pruning with MAX VALUE equality predicate fails

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1766: Java client partition pruning with MAX_VALUE 
equality predicate fails
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5266/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java
File 
java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java:

Line 294: // c < MIN
> Do we have this new coverage in the C++ tests already?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iad6d00d9f401c510e14709323e7686bfa6d1b449
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1766: Java client partition pruning with MAX VALUE equality predicate fails

[Dan Burkert (Code Review)]

Hello Todd Lipcon,

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

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

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

Change subject: KUDU-1766: Java client partition pruning with MAX_VALUE 
equality predicate fails
..

KUDU-1766: Java client partition pruning with MAX_VALUE equality predicate fails

The bug orginated in a mistransliteration of the c++ partition pruning
algorithm; the original version has always had the missing if statement:
https://github.com/apache/kudu/blob/53e67e9eb7b4fc940a14a17119041871e80bcc3f/src/kudu/common/key_util.cc#L152-L155

Change-Id: Iad6d00d9f401c510e14709323e7686bfa6d1b449
---
M java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java
M src/kudu/common/partition_pruner-test.cc
3 files changed, 47 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad6d00d9f401c510e14709323e7686bfa6d1b449
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [c++ client] propagate timestamp for write operations

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: [c++ client] propagate timestamp for write operations
..

[c++ client] propagate timestamp for write operations

Updated the Kudu C++ client library to propagate timestamp for write
operations. This is a fix for

KUDU-1770 C++ client does not propate timestamp for write operations

This patch also enables the integration test which was failing prior
to this fix: ConsistencyITest.TestTimestampPropagationForWriteOps

Change-Id: I17feb6b2dacd0ba7c8b57464f1a50de99de2f772
---
M src/kudu/client/batcher.cc
1 file changed, 10 insertions(+), 3 deletions(-)


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

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

[kudu-CR] bshuf block: some low-hanging-fruit optimizations on write path

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: bshuf_block: some low-hanging-fruit optimizations on write path
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5195/5/src/kudu/cfile/bshuf_block.h
File src/kudu/cfile/bshuf_block.h:

Line 174: return kHeaderSize + count_ * size_of_type;
Why don't we round up this estimate anymore?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia895f7731e5371967782ef9cb176a9d493894a83
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] Update debug partition and row printing

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Update debug partition and row printing
..


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/5262/1/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:

PS1, Line 585:   if (partition.hash_buckets_.size() != 
hash_bucket_schemas_.size()) {
 : return "";
 :   }
should this be a CHECK? would be indicative of programmer error, right? or 
perhaps a LOG(DFATAL) at least so we'd catch it in test builds?


Line 621:   components.emplace_back(Substitute("", 
s.ToString()));
same


Line 652: return "";
same


PS1, Line 689: oakc
not following this reference


Line 706: return "";
same


Line 810: ColumnIdsToColumnNames(schema, 
hash_bucket_schema.column_ids));
for these HTML output functions, we should be escaping the user-provided column 
names


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files
..


Patch Set 19:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/4995/19/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:

Line 556: 
> spurious newline
Done


Line 618:   ts = ts == Timestamp::kInvalidTimestamp ? 
Timestamp(clock_->GetCurrentTime()) : ts;
> how about:
Done


PS19, Line 670: In the end make sure that the output is correct.
> How about: Repeatedly merge all the generated RowSets until we are left wit
Done


Line 699: for (int j = 0; j < num_rowsets_in_layer; ++j) {
> Mind adding a comment about the purpose of this "layers" logic?
I did to the header. having overlapping layers is so that we have stuff to 
compact together. having one less row set per layer is so that we'll likely 
always have rows that are unique in a compaction input which is more "normal" 
than the artificial scenario of having all rowsets but the bottom one have only 
duplicated rows.


Line 715:   // For odd rows, update them and delete them in the drs
> Is there any particular reason why the odd rows in the DRS are updated with
no just the way this test worked previously (look at insert/update row). Done


PS19, Line 717: row_idx++
> Increment row_idx on a separate line for readability
Done


Line 724: // For the next layer remove one rowset worth or rows at random
> needs period
Done


PS19, Line 724: or
> of
Done


Line 727:   row_ids.erase(row_ids.begin() + to_remove);
> expensive operation, isn't it?
this test runs in 374 ms total, my guess is that 90% of that time is spent on 
writes, i don't think that's an issue.


Line 762: // Merge between 2 and 4 row sets
> and here
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
Gerrit-PatchSet: 19
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] bshuf block: some code cleanup

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: bshuf_block: some code cleanup
..


Patch Set 6:

(1 comment)

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

PS6, Line 152:   uint32_t value = array[i];
 :   memcpy([i * sizeof(uint32_t)], , 
sizeof(value));
> nit: I missed that while doing the review, but memmove() might be a little 
I don't get the same results you did, and also typically memcpy is faster than 
memmove because it is able to restrict that the src/dst don't overlap. Let's 
leave as is unless you can show in a macro-benchmark that it matters.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
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: Yes

[kudu-CR] Don't output unobservable rows from the MemRowset

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Don't output unobservable rows from the MemRowset
..

Don't output unobservable rows from the MemRowset

In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.

The sequence is as follows:

Insert Row at 1 -> Row goes in the MRS
Flush   -> Row now lives in RS1
Delete Row at 3 -> Row is now a ghost in RS1
Insert Row at 3 -> Row goes in the MRS again
Delete Row at 3 -> Row is deleted from the MRS
Flush   -> Row is now a ghost in RS2
Major delta compact RS1 -> GC all before 3
Major delta compact RS2 -> GC all before 3

If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.

This adds a small test to compaction-test that makes sure that
a row that is inserted and deleted in the same transaction doesn't
appear in the compaction input.

Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 88 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/14
-- 
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] encodings: change IsBlockFull() to not take a limit parameter

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: encodings: change IsBlockFull() to not take a limit parameter
..

encodings: change IsBlockFull() to not take a limit parameter

Everywhere we called this, we used the same value (the configured cfile
block size). Given that, it's simpler to not take the parameter. This
makes it easier for an encoder to determine whether or not it is full in
some other context, since it can safely assume that it knows the block
size up front.

Change-Id: I4598c1555732649eeebf43231082dd641ecfc576
---
M src/kudu/cfile/binary_dict_block.cc
M src/kudu/cfile/binary_dict_block.h
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/cfile/binary_plain_block.h
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/binary_prefix_block.h
M src/kudu/cfile/block_encodings.h
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/gvint_block.cc
M src/kudu/cfile/gvint_block.h
M src/kudu/cfile/plain_bitmap_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
M src/kudu/cfile/type_encodings.cc
16 files changed, 60 insertions(+), 48 deletions(-)


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

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

[kudu-CR] WIP: KUDU-1767. Create a client flush interleave test

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: WIP: KUDU-1767. Create a client flush interleave test
..


Patch Set 2:

(7 comments)

It looks like a nice test to isolate this problem!

http://gerrit.cloudera.org:8080/#/c/5256/2/src/kudu/integration-tests/client_flush_interleave-itest.cc
File src/kudu/integration-tests/client_flush_interleave-itest.cc:

Line 17: 
nit: consider adding

#include 

#incldue 
#include 


PS2, Line 44: / Check that attempts to scan prior to the ancient history mark 
fail
The comment looks outdated.


PS2, Line 71: KuduUpdate* update
nit: consider using unique_ptr to wrap the update op in case of any of the 
ASSERT_OK() below fail (I suspect that could lead to ASAN warnings)


PS2, Line 76:   for (int i = 0; i < kMax; i++) {
: ASSERT_OK(session->Apply(updates[i]));
: session->FlushAsync(nullptr);
:   }
if using unique_ptr, probably you could use deque instead of vector or inserts 
those updates in reverse order and then do pop_back() if using the vector 
container.


PS2, Line 91: LOG(INFO)
Does it make sense to use VLOG() here instead?  Or it's crucial to have those 
lines always printed out?


PS2, Line 97: "int_val=$0"
What do those lines look like?  My concern is that some portion of anomalies 
could be missed here; e.g., consider

'int_val=1' and 'int_val=10' and alike.

May be, it's worth to update the check to make sure those cases are also 
covered.


PS2, Line 101:   /*
 :   KuduScanner scanner(table.get());
 :   ASSERT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT));
 :   Status s = scanner.Open();
 :   ASSERT_TRUE(s.IsInvalidArgument()) << s.ToString();
 :   ASSERT_STR_CONTAINS(s.ToString(), "Snapshot timestamp is 
earlier than the ancient history mark");
 :   */
nit: consider removing this commented code.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3886f70246c6ef3f94acda6074d02d4fb199cf50
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] bshuf block: some code cleanup

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: bshuf_block: some code cleanup
..


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/5193/3/src/kudu/cfile/bshuf_block.cc
File src/kudu/cfile/bshuf_block.cc:

PS3, Line 59: uint32_t value = *cell_ptr(i);
: max_value = std::max(max_value, value);
> nit: may be, drop the local variable:
Done


PS3, Line 152: int
> hopefully, 'n' cannot be too big
yea, we'd never have >2B entries in a block


PS3, Line 154: implicit_cast
> why cast is needed here at all?  This is an unsigned type, so direct assign
Done


PS3, Line 160: implicit_cast
> ditto: I don't think cast is needed here.
Done


http://gerrit.cloudera.org:8080/#/c/5193/3/src/kudu/cfile/bshuf_block.h
File src/kudu/cfile/bshuf_block.h:

PS3, Line 155: int
> nit: why int, not uint32_t ?
google style guide says not to use unsigned types except to represent bit 
patterns or wire formats, etc. A lot of this code is old from before we started 
following that rule, which is why there's a bit of inconsistency.

Updated this bit of code to be clearer anyway.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
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: Yes

[kudu-CR] Update debug partition and row printing

[Dan Burkert (Code Review)]

Hello Adar Dembo, Dimitris Tsirogiannis, Todd Lipcon,

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

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

to review the following change.

Change subject: Update debug partition and row printing
..

Update debug partition and row printing

This commit updates the debug printing of rows and partitions in
metrics, log messages, and web UIs. The new format is one we have
settled on after consulting with committers on Apache Impala. In all
cases I think it makes it easier to work with complex partitions. The
most invasive aspect of this change is that string and binary column
values are now printed with quotes, which affects a bunch of otherwise
unrelated tests.

I attempted to minimize as much of the duplication as possible in debug
formatting functions in partition.cc, but there is still a lot of
duplication that was unavoidable (this was true before making this
change as well).

Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/common/encoded_key-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/common/row_changelist-test.cc
M src/kudu/common/row_operations-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/schema-test.cc
M src/kudu/common/types.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/composite-pushdown-test.cc
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/tablet-pushdown-test.cc
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/tserver/tablet_server-test.cc
25 files changed, 479 insertions(+), 449 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] bshuf block: some code cleanup

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: bshuf_block: some code cleanup
..

bshuf_block: some code cleanup

* Some typo/grammar fixes/reformatting in comments.
* Rename kMaxHeaderSize to kHeaderSize since the header is fixed-length.
* Adds a private cell_ptr() function instead of error prone indexing
  with multiplication into the data_ buffer.
* Remove an unnecessarily UINT32 specialization
* Fix some C-style casts and unnecessary type punning in favor of
  memcpy()
* Cleaned up padding code to use KUDU_ALIGN_UP

This also adds a TODO for a bug with GetLastKey: we are mistakenly
indexing into data_[count - 1] instead of data_[(count - 1) *
size_of_type] which causes incorrect results. The fix for this is in the
following commit.

Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
---
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
2 files changed, 88 insertions(+), 79 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6857f8096319072eb09be097adea99c45735e0a6
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
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 

[kudu-CR] tablet copy: Rename tablet copy session source files

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: tablet copy: Rename tablet copy session source files
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I23da316bc3c5d5de412d2d86b1bc7ee77ef60be5
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tablet copy: Rename tablet copy session source files

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: tablet copy: Rename tablet copy session source files
..


tablet copy: Rename tablet copy session source files

This is just a quick cleanup to the class rename in the previous commit.

Change-Id: I23da316bc3c5d5de412d2d86b1bc7ee77ef60be5
Reviewed-on: http://gerrit.cloudera.org:8080/5042
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/tablet_copy_service.cc
R src/kudu/tserver/tablet_copy_source_session-test.cc
R src/kudu/tserver/tablet_copy_source_session.cc
R src/kudu/tserver/tablet_copy_source_session.h
5 files changed, 5 insertions(+), 5 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I23da316bc3c5d5de412d2d86b1bc7ee77ef60be5
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tablet copy: Fix tidy warnings in TabletCopySourceSession{Test}

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: tablet copy: Fix tidy warnings in TabletCopySourceSession{Test}
..


tablet copy: Fix tidy warnings in TabletCopySourceSession{Test}

Done as a follow-up commit to allow for a clean rename in the previous
commit.

Change-Id: I5b9dd173a8c0a7118225a5b4f24ae905c98e3c27
Reviewed-on: http://gerrit.cloudera.org:8080/5257
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/tablet_copy_source_session.cc
2 files changed, 5 insertions(+), 12 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I5b9dd173a8c0a7118225a5b4f24ae905c98e3c27
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [i-tests] scan token timestamp propagation test

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:

Line 591: ASSERT_OK(builder.SetSnapshotRaw(ts));
> maybe I'm missing something: if the scan tokens are propagating timestamps 
Yes, it seems I see your point.  Right, that's the essence of the timestamp 
propagation.

May be, I misunderstood Dan's comment -- I thought Dan asked whether the 
timestamp for the token-to-be is assigned implicitly from the client's latest 
observed one, i.e. whether the generated token would have the timestamp set 
even if we didn't set it explicitly to any value.  In my comment I addressed 
that thing.  That's because I saw some sort of parallel in here: check 
scanner-internal.cc, line 423. 

OK, it seems it's just my misunderstanding.  I'll remove the timestamp 
assignment above.


PS1, Line 606: shared_ptr client;
 : ASSERT_OK(cluster_->CreateClient(nullptr, ));
 : shared_ptr table;
 : ASSERT_OK(client->OpenTable(table_name_, ));
 : KuduScanner* scanner_raw;
 : 
ASSERT_OK(KuduScanToken::DeserializeIntoScanner(client.get(), scan_token,
 : 
_raw));
 : unique_ptr scanner(scanner_raw);
 : scanner->SetTimeoutMillis(scan_timeout_msec);
 : ASSERT_OK(scanner->Open());
 : size_t row_num = 0;
 : while (scanner->HasMoreRows()) {
 :   KuduScanBatch batch;
 :   ASSERT_OK(scanner->NextBatch());
 :   row_num += batch.NumRows();
 : }
 : EXPECT_EQ(0, row_num);
> the trickyness stems from the fact that you could get timeouts that are unr
ok, I see; that makes sense.  I could not see other way of checking that.  Will 
need to explore other alternatives around.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
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: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] Update debug partition and row printing

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: Update debug partition and row printing
..


Patch Set 1:

screenshots of updated web UIs: http://imgur.com/a/VNlMA

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] cmake: Throw cmake error if unit test does not exist

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: cmake: Throw cmake error if unit test does not exist
..


cmake: Throw cmake error if unit test does not exist

Previously, if the unit test file did not exist, CMake would not notice
and the problem was difficult to debug at test runtime.

Change-Id: I6c7d048f44624c8406198a098e0a7ecb44f8a08e
Reviewed-on: http://gerrit.cloudera.org:8080/5222
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M CMakeLists.txt
1 file changed, 3 insertions(+), 1 deletion(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6c7d048f44624c8406198a098e0a7ecb44f8a08e
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] cmake: Throw cmake error if unit test does not exist

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: cmake: Throw cmake error if unit test does not exist
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6c7d048f44624c8406198a098e0a7ecb44f8a08e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] cfile set-test: switch to using INT32 instead of UINT32

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: cfile_set-test: switch to using INT32 instead of UINT32
..


cfile_set-test: switch to using INT32 instead of UINT32

We don't actually support users using UINT32 anymore, so it's better to
test the exposed INT32 type.

Change-Id: I48dd5e81c1cd725328ea96b030279d59a1c845a5
Reviewed-on: http://gerrit.cloudera.org:8080/5194
Reviewed-by: Dan Burkert 
Tested-by: Kudu Jenkins
---
M src/kudu/tablet/cfile_set-test.cc
1 file changed, 20 insertions(+), 20 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I48dd5e81c1cd725328ea96b030279d59a1c845a5
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] encodings: change IsBlockFull() to not take a limit parameter

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: encodings: change IsBlockFull() to not take a limit parameter
..


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/5200/2/src/kudu/cfile/binary_plain_block.cc
File src/kudu/cfile/binary_plain_block.cc:

Line 86: // TODO: does it cost a lot to account these things specifically?
> warning: missing username/bug in TODO [google-readability-todo]
Done


http://gerrit.cloudera.org:8080/#/c/5200/2/src/kudu/cfile/binary_prefix_block.cc
File src/kudu/cfile/binary_prefix_block.cc:

Line 83:   // TODO: take restarts size into account
> warning: missing username/bug in TODO [google-readability-todo]
Done


http://gerrit.cloudera.org:8080/#/c/5200/2/src/kudu/cfile/gvint_block.h
File src/kudu/cfile/gvint_block.h:

Line 52:   int Add(const uint8_t *vals, size_t count) OVERRIDE;
> warning: function 'kudu::cfile::GVIntBlockBuilder::Add' has a definition wi
Done


http://gerrit.cloudera.org:8080/#/c/5200/2/src/kudu/cfile/plain_bitmap_block.h
File src/kudu/cfile/plain_bitmap_block.h:

Line 52: return writer_.bytes_written() > 
options_->storage_attributes.cfile_block_size;
> error: member access into incomplete type 'const kudu::cfile::WriterOptions
Done


http://gerrit.cloudera.org:8080/#/c/5200/2/src/kudu/cfile/rle_block.h
File src/kudu/cfile/rle_block.h:

Line 69: return encoder_.len() > 
options_->storage_attributes.cfile_block_size;
> error: member access into incomplete type 'const kudu::cfile::WriterOptions
Done


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

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

[kudu-CR] [i-tests] scan token timestamp propagation test

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:

Line 591: ASSERT_OK(builder.SetSnapshotRaw(ts));
> Currently that's not the case -- the ScanConfiguration object underneath th
maybe I'm missing something: if the scan tokens are propagating timestamps 
correctly and we don't set a timestamp on the snapshot scan then the server 
should choose a timestamp that is higher than the last write. that is the point 
of timestamp propagation in scan tokens no?


PS1, Line 606: shared_ptr client;
 : ASSERT_OK(cluster_->CreateClient(nullptr, ));
 : shared_ptr table;
 : ASSERT_OK(client->OpenTable(table_name_, ));
 : KuduScanner* scanner_raw;
 : 
ASSERT_OK(KuduScanToken::DeserializeIntoScanner(client.get(), scan_token,
 : 
_raw));
 : unique_ptr scanner(scanner_raw);
 : scanner->SetTimeoutMillis(scan_timeout_msec);
 : ASSERT_OK(scanner->Open());
 : size_t row_num = 0;
 : while (scanner->HasMoreRows()) {
 :   KuduScanBatch batch;
 :   ASSERT_OK(scanner->NextBatch());
 :   row_num += batch.NumRows();
 : }
 : EXPECT_EQ(0, row_num);
> What's exactly tricky?  I don't see any trickiness here -- please check my 
the trickyness stems from the fact that you could get timeouts that are 
unrelated with what you are testing and that the test is only doing an indirect 
check. My comment was: can you do direct check somewhere? Its cool if not 
possible but if it is it would be preferable


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
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: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] Don't output unobservable rows from the MemRowset

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Don't output unobservable rows from the MemRowset
..


Patch Set 13:

> 
 > DRS 1:
 > undo: del @ 2
 > base: v1
 > redo delta: del @3
 > 
 > DRS 2:
 > base: v2
 > 
 > 
 > 
 > flush and compact
 > ---
 > 
 > ?

I think in this case you would still have a del UNDO @ 3 in DRS 2, so you could 
reason that at the same TS a del UNDO in a higher RS comes after a del REDO in 
a lower RS. Right?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-798 (part 2) Remove automatic safe time adjustment from mvcc

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-798 (part 2) Remove automatic safe time adjustment from 
mvcc
..


Patch Set 13: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6cd0b8aa52e4b0f5c0f4872d806e76387f6b9538
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1757: fix appendCellValueDebugString, do not throw exception when a column is not set

[YanlongZheng (Code Review)]

YanlongZheng has posted comments on this change.

Change subject: KUDU-1757: fix appendCellValueDebugString, do not throw 
exception when a column is not set
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5237/1/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
File java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java:

PS1, Line 597:  return;
 : }
 : 
 :   
> I think it's better to just not include it in the string, rather than inclu
Currently we have two kinds of debug string: debug string and short debug 
string. If not include it, the outputs are like:
(for example, key2 is not set)
debug string:int key1=123, int key2=, int val1=456, ...
short debug string:  123, , 456, ...

Personally, I think these are better:
debug string:int key1=234, int key2=(not set), int val1=456, ...
short debug string:  123, (not set), 456, ...

Could you please consider a little more and make the final decision? Then I 
will make the change.


http://gerrit.cloudera.org:8080/#/c/5237/2/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
File java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java:

PS2, Line 596: {
 :   return;
 : }
Currently we have two kinds of debug string: debug string and short debug 
string. If not include any information, the outputs are like:
(for example, key2 is not set)
debug string:int key1=123, int key2=, int val1=456, ...
short debug string:  123, , 456, ...
Personally, I think these are better:
debug string:int key1=234, int key2=(not set), int val1=456, ...
short debug string:  123, (not set), 456, ...
Could you please consider a little more and make the final decision? Then I 
will make the change.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife05af5a11e95751b3cc259e786d36494aeb5d3a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: YanlongZheng 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: YanlongZheng 
Gerrit-HasComments: Yes

[kudu-CR] Add Reinserts to tablet history gc-itest

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Add Reinserts to tablet_history_gc-itest
..


Patch Set 13: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [c++] ColumnPredicate simplification for inequalities on boundary values

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [c++] ColumnPredicate simplification for inequalities on 
boundary values
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5273/1//COMMIT_MSG
Commit Message:

PS1, Line 11: transormations
> transformations
Done


PS1, Line 16: mutliple
> multiple
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9bb150249a51a69fb85af67d34e85b7d7bed325c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add snapshot scans to fuzz-itest

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add snapshot scans to fuzz-itest
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:

PS12, Line 574: down_cast(
  : tablet()->clock().get())
> How do you guarantee that all path which you code will see in release mode 
you mean in this test? how could it be different?

In any case if you look throughout the whole code base this is _always_ how we 
do it. there isn't a single instance of dynamic_cast and 16 instances of 
down_cast.

I remain unconvinced that this test, in which we choose the clock 
implementation on start up, should be the exception. Particularly as if you 
change the clock implementation on set up and not the the down_cast the build 
would fail.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Support safe time advancement on replicas in the absense of writes

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: Support safe time advancement on replicas in the absense 
of writes
..

WIP: Support safe time advancement on replicas in the absense of writes

Mostly getting some jenkins mileage but the design should be close
to the final one.

This is a WIP that needs to be broken down further but that summarily
adds the following:

- Add a TimeManager class: This class is responsible for tracking
  safe time advancement on leaders and replicas.
- Removes the clock from MvccManager - Since safe time is now managed
  by the TimeManager there is no need for Mvcc to have one internally.
- Changes how we wait for "all committed" in MvccManager. Previously
  we would wait for a clean snapshot. Now we instead wait for all
  transactions before the timestamp to be committed even if the
  clean timestamp doesn't advance. This is because without leader
  leases the safe time can go back, but moving the clean time back
  would break a lot of things internally.
- Plugs the TimeManager in consensus/queue and calls its methods
  appropriately.

Change-Id: I8532fdb069c8bee7f3e08ffe74cab0273885cc8e
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus.h
M src/kudu/consensus/consensus.proto
M src/kudu/consensus/consensus_peers-test.cc
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
A src/kudu/consensus/time_manager-test.cc
A src/kudu/consensus/time_manager.cc
A src/kudu/consensus/time_manager.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_peer.cc
M src/kudu/tablet/tablet_peer.h
M src/kudu/tablet/transactions/alter_schema_transaction.cc
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
31 files changed, 589 insertions(+), 176 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8532fdb069c8bee7f3e08ffe74cab0273885cc8e
Gerrit-PatchSet: 7
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 

[kudu-CR] Update debug partition and row printing

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Update debug partition and row printing
..

Update debug partition and row printing

This commit updates the debug printing of rows and partitions in
metrics, log messages, and web UIs. The new format is one we have
settled on after consulting with committers on Apache Impala. In all
cases I think it makes it easier to work with complex partitions. The
most invasive aspect of this change is that string and binary column
values are now printed with quotes, which affects a bunch of otherwise
unrelated tests.

I attempted to minimize as much of the duplication as possible in debug
formatting functions in partition.cc, but there is still a lot of
duplication that was unavoidable (this was true before making this
change as well).

Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/common/encoded_key-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/common/row_changelist-test.cc
M src/kudu/common/row_operations-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/schema-test.cc
M src/kudu/common/types.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/composite-pushdown-test.cc
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/tablet-pushdown-test.cc
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/tserver/tablet_server-test.cc
25 files changed, 505 insertions(+), 470 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-237 (part 2) - Add support for REINSERT in delta files

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-237 (part 2) - Add support for REINSERT in delta files
..


Patch Set 24: Code-Review+2

just a rebase keeping todd's +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1173b2bea721b376f2b6049be20f57307582c47
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add snapshot scans to fuzz-itest

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add snapshot scans to fuzz-itest
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:

PS12, Line 574: down_cast(
  : tablet()->clock().get())
> you mean in this test? how could it be different?
Thank you for the clarification.

My question was more about generic approach to justify usage of the down_cast<> 
code: to use that down_cast<> it's better to be 100% sure the release mode 
builds will see the same control paths that debug mode builds see and all those 
path are validated.  I don't know how to get 100% assurance on that in general. 
 Frankly, I don't think it's even possible to provide such a guarantee.  That's 
why I think using down_cast<> is dangerous. :)

As for this test, since it's scope is confined, I don't feel strong against 
using down_cast<> -- it's all up to you.  I just wanted to understand what was 
the rationale behind using down_cast<>


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Support safe time advancement on replicas in the absense of writes

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: Support safe time advancement on replicas in the absense 
of writes
..

WIP: Support safe time advancement on replicas in the absense of writes

Mostly getting some jenkins mileage but the design should be close
to the final one.

This is a WIP that needs to be broken down further but that summarily
adds the following:

- Add a TimeManager class: This class is responsible for tracking
  safe time advancement on leaders and replicas.
- Removes the clock from MvccManager - Since safe time is now managed
  by the TimeManager there is no need for Mvcc to have one internally.
- Changes how we wait for "all committed" in MvccManager. Previously
  we would wait for a clean snapshot. Now we instead wait for all
  transactions before the timestamp to be committed even if the
  clean timestamp doesn't advance. This is because without leader
  leases the safe time can go back, but moving the clean time back
  would break a lot of things internally.
- Plugs the TimeManager in consensus/queue and calls its methods
  appropriately.

Change-Id: I8532fdb069c8bee7f3e08ffe74cab0273885cc8e
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus.h
M src/kudu/consensus/consensus.proto
M src/kudu/consensus/consensus_peers-test.cc
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
A src/kudu/consensus/time_manager-test.cc
A src/kudu/consensus/time_manager.cc
A src/kudu/consensus/time_manager.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_peer.cc
M src/kudu/tablet/tablet_peer.h
M src/kudu/tablet/transactions/alter_schema_transaction.cc
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
31 files changed, 587 insertions(+), 176 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8532fdb069c8bee7f3e08ffe74cab0273885cc8e
Gerrit-PatchSet: 6
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 

[kudu-CR] Don't output unobservable rows from the MemRowset

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Don't output unobservable rows from the MemRowset
..


Patch Set 17: Code-Review+1

(5 comments)

just a rebase, keeping todd's +1

http://gerrit.cloudera.org:8080/#/c/4994/13/src/kudu/tablet/compaction-test.cc
File src/kudu/tablet/compaction-test.cc:

Line 618:   {
> Is it important that we do not roll?
yeah, we want a single rowset, not that these rows would fill one, but this way 
we're sure and we dont have to make assertion on the number of the resulting 
rowsets.


http://gerrit.cloudera.org:8080/#/c/4994/13/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:

Line 113: // Get the latest mutation.
> Consider writing this like:
Done.
Its unlikely that the we have both a redo head and that it's timesatmp is the 
same as the insertion timestamp, right.
If for nothing else I think it has documentation value.


Line 114: const Mutation* latest = input_row.redo_head;
> nit: punctuation
Done


Line 117: latest->timestamp() == insertion_timestamp) {
> similarly, consider writing this like:
Done


Line 134:   block->resize(next_row_index);
> Maybe better written as:
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add snapshot scans to fuzz-itest

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add snapshot scans to fuzz-itest
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:

PS12, Line 574: down_cast(
  : tablet()->clock().get())
> thats a google style recommendation. we also build the clock inside the tes
Ah, I see -- so it's a code style thing.

The danger of the static_cast<> while doing downcasting: it does not allow to 
see that the operand could not be downcasted to the expected type.  It's 
possible to end-up in a situation when the code executes some other methods, 
not what you expect.  The comment for that down_cast<> method does not make any 
sense to me at all, since I don't understand how one can guarantee that all the 
paths which the code is about to execute are covered with the debug mode runs.  
So, I would either forbid any downcasting at all or allow it and make sure it's 
done with dynamic_cast<> only, so it's safe.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java-client] KuduPredicate simplification for inequalities on boundary values

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java-client] KuduPredicate simplification for inequalities on 
boundary values
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5272/1//COMMIT_MSG
Commit Message:

PS1, Line 11: transormations
transformations


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id3779029d95532e91b235b4ed33a77e2a32ad072
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [c++] ColumnPredicate simplification for inequalities on boundary values

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [c++] ColumnPredicate simplification for inequalities on 
boundary values
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9bb150249a51a69fb85af67d34e85b7d7bed325c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add snapshot scans to fuzz-itest

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add snapshot scans to fuzz-itest
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:

PS12, Line 574: down_cast(
  : tablet()->clock().get())
> well if you look at the down_cast definition in casts.h you'll see that in 
How do you guarantee that all path which you code will see in release mode are 
the same which it sees in debug mode?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add snapshot scans to fuzz-itest

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add snapshot scans to fuzz-itest
..


Patch Set 16: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] cfile: make encoder/decoder classes final

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: cfile: make encoder/decoder classes final
..

cfile: make encoder/decoder classes final

Marking the classes final means that the compiler can be sure that a
virtual method call from within the class will always be satisfied by
the implementation in that same class (since no subclass could exist).

This increases inlining opportunities and other downstream
optimizations, and should improve performance a few percent in various
spots.

I verified the effect by running:

  perf record  ./build/latest/bin/cfile-test 
--gtest_filter=\*100MFileStringsPlain\*0
  perf report -n | grep BinaryPlainBlockBuilder | c++filt

Before:
  Overhead   Samples  Command  Shared ObjectSymbol
 6.22%  1943  cfile-test   cfile-test   [.] 
kudu::cfile::BinaryPlainBlockBuilder::Add(unsigned char const*, unsigned long)
 1.23%   386  cfile-test   cfile-test   [.] 
kudu::cfile::BinaryPlainBlockBuilder::IsBlockFull() const
 0.23%73  cfile-test   cfile-test   [.] 
kudu::cfile::BinaryPlainBlockBuilder::Finish(unsigned int)
 0.00% 1  cfile-test   cfile-test   [.] 
kudu::cfile::BinaryPlainBlockBuilder::Reset()

After:
  Overhead   Samples  Command  Shared ObjectSymbol
 6.21%  1868  cfile-test   cfile-test   [.] 
kudu::cfile::BinaryPlainBlockBuilder::Add(unsigned char const*, unsigned long)
 0.23%69  cfile-test   cfile-test   [.] 
kudu::cfile::BinaryPlainBlockBuilder::Finish(unsigned int)

You can see that several functions were now inlined where they previously could
not be, and the sample count for 'Add' was also reduced.

Change-Id: Ia0b8337868330b43595a275e837a448e3ba0c066
---
M src/kudu/cfile/binary_dict_block.h
M src/kudu/cfile/binary_plain_block.h
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/binary_prefix_block.h
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/gvint_block.h
M src/kudu/cfile/plain_bitmap_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
9 files changed, 19 insertions(+), 22 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia0b8337868330b43595a275e837a448e3ba0c066
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Remove GROUP VARINT encoding

[Todd Lipcon (Code Review)]

Hello Dan Burkert,

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

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

to review the following change.

Change subject: Remove GROUP_VARINT encoding
..

Remove GROUP_VARINT encoding

This encoding was only ever implemented for the UINT32 type, and users
can no longer specify this type, since we only support signed ints.
Thus, there's no sense carrying around this baggage.

This patch removes the encoding as well as associated tests.

Change-Id: I0b0aeb5bfd8451a6e190ccb6e55e971da668b062
---
M src/kudu/cfile/CMakeLists.txt
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/cfile/encoding-test.cc
D src/kudu/cfile/gvint_block.cc
D src/kudu/cfile/gvint_block.h
M src/kudu/cfile/type_encodings.cc
M src/kudu/client/schema.h
M src/kudu/common/common.proto
10 files changed, 11 insertions(+), 626 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0b0aeb5bfd8451a6e190ccb6e55e971da668b062
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] Add snapshot scans to fuzz-itest

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add snapshot scans to fuzz-itest
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:

PS12, Line 574: down_cast(
  : tablet()->clock().get())
> I don't mind using down_cast<> here, but what is the rational behind not us
thats a google style recommendation. we also build the clock inside the test 
and this wouldn't work with the hybrid clock so what would be the danger here?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] binary plain block: avoid a virtual call in hot path

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: binary_plain_block: avoid a virtual call in hot path
..


Patch Set 3: Code-Review-1

Dan and I actaully just chatted, and it sounds like a better solution with 
equivalent perf results may be to mark these encoder classes as 'final', so the 
compiler knows it can devirtualize such calls.

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

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

[kudu-CR] Add snapshot scans to fuzz-itest

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add snapshot scans to fuzz-itest
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4996/12/src/kudu/integration-tests/fuzz-itest.cc
File src/kudu/integration-tests/fuzz-itest.cc:

PS12, Line 574: down_cast(
  : tablet()->clock().get())
> we shouldn't use dynamic_cast on non-debug builds
I don't mind using down_cast<> here, but what is the rational behind not using 
dynamic_cast in non-debug builds?

As I can see, the down_cast<> is way more dangerous than dynamic_cast<> -- it's 
reduced just to static_cast<> in non-debug builds, which could lead to 
completely unpredictable results, way worse than a penalty of doing some 
RTTI-related calls.  Also, this is just test code, so I wouldn't expect much of 
performance optimizations to be in place.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d15129e83c5c9afa9b643e674c8a23e18a2fa0e
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Don't output unobservable rows from the MemRowset

[David Ribeiro Alves (Code Review)]

Hello Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: Don't output unobservable rows from the MemRowset
..

Don't output unobservable rows from the MemRowset

In some rare cases we might have a row that is inserted and deleted
in the same operation, meaning the insert and delete have the same
timestamp. In this rare case we might run into trouble later on with
compactions as there is a sequence of operations that that produce
two ghost rows which are uncomparable in terms of recency.

The sequence is as follows:

Insert Row at 1 -> Row goes in the MRS
Flush   -> Row now lives in RS1
Delete Row at 3 -> Row is now a ghost in RS1
Insert Row at 3 -> Row goes in the MRS again
Delete Row at 3 -> Row is deleted from the MRS
Flush   -> Row is now a ghost in RS2
Major delta compact RS1 -> GC all before 3
Major delta compact RS2 -> GC all before 3

If RS1 and RS2 now get compacted together there is no way to
distinguish Ghost 1 from Ghost 2 and to build a correct history
for the row.

The point is that we can't trust UNDOs to break ties, in terms of
recency, between two ghosts as they might have been removed by
delta compactions. However we can always trust REDO delete/reinsert
to remain.

This adds a small test to compaction-test that makes sure that
a row that is inserted and deleted in the same transaction doesn't
appear in the compaction input.

FuzzTest and TestRandomAccess would fail for the following patch
(KUDU-237 (part 2)) without this change.

Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
---
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/compaction.cc
2 files changed, 185 insertions(+), 62 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/4994/16
-- 
To view, visit http://gerrit.cloudera.org:8080/4994
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [i-tests] scan token timestamp propagation test

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:

PS1, Line 611: ASSERT_OK(KuduScanToken::DeserializeIntoScanner(client.get(), 
scan_token,
 : 
_raw));
> It could be a good solution, however the scan below does not succeed, so th
ok, it seems I see what you mean: that's what  we should see in case of 
success.  If it fails, it fails and we don't do any conclusions -- that's the 
essence.

All right, this sounds good to me.  Will update in a moment.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
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: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] Add Reinserts to tablet history gc-itest

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add Reinserts to tablet_history_gc-itest
..


Patch Set 16: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6b774b14d79bd7c0c0bf62b585bd70d6d975feaa
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] cfile-test: use a faster data generator for 100M-string test

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: cfile-test: use a faster data generator for 100M-string test
..


Patch Set 3:

I don't think it'll be that much less effective, since now it'll at least have 
the prefix '00' or somesuch instead of "hello ", right?

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

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

[kudu-CR] Don't output unobservable rows from the MemRowset

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Don't output unobservable rows from the MemRowset
..


Patch Set 15: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieab52a1aa68494218f91f3acd31ef8ddf352bd57
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP: Support safe time advancement on replicas in the absense of writes

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: Support safe time advancement on replicas in the absense 
of writes
..

WIP: Support safe time advancement on replicas in the absense of writes

Mostly getting some jenkins mileage but the design should be close
to the final one.

This is a WIP that needs to be broken down further but that summarily
adds the following:

- Add a TimeManager class: This class is responsible for tracking
  safe time advancement on leaders and replicas.
- Removes the clock from MvccManager - Since safe time is now managed
  by the TimeManager there is no need for Mvcc to have one internally.
- Changes how we wait for "all committed" in MvccManager. Previously
  we would wait for a clean snapshot. Now we instead wait for all
  transactions before the timestamp to be committed even if the
  clean timestamp doesn't advance. This is because without leader
  leases the safe time can go back, but moving the clean time back
  would break a lot of things internally.
- Plugs the TimeManager in consensus/queue and calls its methods
  appropriately.

Change-Id: I8532fdb069c8bee7f3e08ffe74cab0273885cc8e
---
M src/kudu/consensus/CMakeLists.txt
M src/kudu/consensus/consensus.h
M src/kudu/consensus/consensus.proto
M src/kudu/consensus/consensus_peers-test.cc
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
A src/kudu/consensus/time_manager-test.cc
A src/kudu/consensus/time_manager.cc
A src/kudu/consensus/time_manager.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/diskrowset-test-base.h
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_peer.cc
M src/kudu/tablet/tablet_peer.h
M src/kudu/tablet/transactions/alter_schema_transaction.cc
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
31 files changed, 586 insertions(+), 176 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/40/5240/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5240
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8532fdb069c8bee7f3e08ffe74cab0273885cc8e
Gerrit-PatchSet: 5
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 

[kudu-CR] cfile-test: use a faster data generator for 100M-string test

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: cfile-test: use a faster data generator for 100M-string test
..


Patch Set 3:

This is going to make the prefix encoder less effective on the dataset, is that 
important?

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

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

[kudu-CR] cfile-test: use a faster data generator for 100M-string test

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: cfile-test: use a faster data generator for 100M-string test
..


Patch Set 3: Code-Review+2

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

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

[kudu-CR] Remove GROUP VARINT encoding

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: Remove GROUP_VARINT encoding
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0b0aeb5bfd8451a6e190ccb6e55e971da668b062
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-1766: Java client partition pruning with MAX VALUE equality predicate fails

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1766: Java client partition pruning with MAX_VALUE 
equality predicate fails
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iad6d00d9f401c510e14709323e7686bfa6d1b449
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] cfile-test: use a faster data generator for 100M-string test

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: cfile-test: use a faster data generator for 100M-string test
..


Patch Set 3:

Right, forgot about the padding.

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

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

[kudu-CR] cfile-test: use a faster data generator for 100M-string test

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: cfile-test: use a faster data generator for 100M-string test
..


cfile-test: use a faster data generator for 100M-string test

The current data generator using StringPrintf is not very fast, and
since we don't care about the contents of the file for this test, it's
just as easy to use FastHex64ToBuffer for better performance.

This resulted in a ~2x speedup in the test. This means that the test
will now better represent the underlying performance of the encoder and
cfile writing code.

Change-Id: I9578681aa2e064dbecd79fdce7b8a083ef03b3c4
Reviewed-on: http://gerrit.cloudera.org:8080/5201
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M src/kudu/cfile/cfile-test.cc
1 file changed, 5 insertions(+), 1 deletion(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I9578681aa2e064dbecd79fdce7b8a083ef03b3c4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] bshuf block: some low-hanging-fruit optimizations on write path

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: bshuf_block: some low-hanging-fruit optimizations on write path
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5195/5//COMMIT_MSG
Commit Message:

Line 9: Rather than adding an element at a time and calling the virtual
> gvint_block.cc is doing something very similar.  Perhaps it would be good t
I think gvint is basically dead except for test codepaths, since it's only 
usable for UINT32, and users can't specify uint32 anymore


http://gerrit.cloudera.org:8080/#/c/5195/5/src/kudu/cfile/bshuf_block.h
File src/kudu/cfile/bshuf_block.h:

PS5, Line 93: OVERRIDE
> nit: here and elsewhere in this file: consider replacing the macro with 'ov
would rather do this separately


Line 174: return kHeaderSize + count_ * size_of_type;
> Why don't we round up this estimate anymore?
not worth it to improve estimate by ~30 bytes max


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia895f7731e5371967782ef9cb176a9d493894a83
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
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 
Gerrit-HasComments: Yes

[kudu-CR] cfile: make encoder/decoder classes final

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: cfile: make encoder/decoder classes final
..


Patch Set 4: Code-Review+2

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

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

[kudu-CR] [i-tests] scan token timestamp propagation test

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [i-tests] scan token timestamp propagation test
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5219/1/src/kudu/integration-tests/consistency-itest.cc
File src/kudu/integration-tests/consistency-itest.cc:

PS1, Line 611: ASSERT_OK(KuduScanToken::DeserializeIntoScanner(client.get(), 
scan_token,
 : 
_raw));
> how about here we assert that the lot here is 'ts' from the block above and
It could be a good solution, however the scan below does not succeed, so this 
would not work.

Or some other changes are needed as well?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47cd067248f4a26c4605f075ec5ee30da71f6f30
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: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1766: Java client partition pruning with MAX VALUE equality predicate fails

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: KUDU-1766: Java client partition pruning with MAX_VALUE 
equality predicate fails
..


KUDU-1766: Java client partition pruning with MAX_VALUE equality predicate fails

The bug orginated in a mistransliteration of the c++ partition pruning
algorithm; the original version has always had the missing if statement:
https://github.com/apache/kudu/blob/53e67e9eb7b4fc940a14a17119041871e80bcc3f/src/kudu/common/key_util.cc#L152-L155

Change-Id: Iad6d00d9f401c510e14709323e7686bfa6d1b449
Reviewed-on: http://gerrit.cloudera.org:8080/5266
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java
M src/kudu/common/partition_pruner-test.cc
3 files changed, 47 insertions(+), 3 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iad6d00d9f401c510e14709323e7686bfa6d1b449
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to 
".kudutmp"
..

KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"

As these ".tmp" strings were scattered over the code, I've tried to remove
this duplication as well by defining the single kTmpInfix in path_util.h.
In a couple of places it led to string concatenations, but I suppose it's
not that bad for overall performance?
The necessity of moving from "tmp" to "kudutmp" is backed by the idea that
we don't want to remove any user's data accidentally. For example, we may
have a symlink to some external directory, which doesn't belong only to
Kudu. Hence, we don't want to delete any ordinary tmp files from it.
(see https://gerrit.cloudera.org/#/c/5007/).

Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
---
M src/kudu/consensus/log.cc
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
M src/kudu/util/pb_util.cc
10 files changed, 25 insertions(+), 22 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to ".kudutmp"

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1634 (part 3). Changed tmp files' infix from ".tmp" to 
".kudutmp"
..


Patch Set 4:

> Hey Maxim, looks like this patch has some conflicts and needs to be
 > rebased.

Don't worry about it; I rebased the patch.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] bshuf block: some low-hanging-fruit optimizations on write path

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: bshuf_block: some low-hanging-fruit optimizations on write path
..


bshuf_block: some low-hanging-fruit optimizations on write path

Rather than adding an element at a time and calling the virtual
'IsBlockFull()' function, we predetermine how many elements we will
accept. This allows a much simpler batched 'Add()' implementation.

This sped up the write side of cfile-test's 100M-integer test about 2x.

Change-Id: Ia895f7731e5371967782ef9cb176a9d493894a83
Reviewed-on: http://gerrit.cloudera.org:8080/5195
Reviewed-by: Dan Burkert 
Tested-by: Kudu Jenkins
---
M src/kudu/cfile/bshuf_block.h
1 file changed, 10 insertions(+), 26 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia895f7731e5371967782ef9cb176a9d493894a83
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
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