date:20160924

[kudu-CR] [tests] fix test which fails with two cpus and document other dependencies

2016-09-24 Thread Brock Noland (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [tests] fix test which fails with two cpus and document other 
dependencies
..

[tests] fix test which fails with two cpus and document other dependencies

Various tests fail due to lack of lsof and low resource
limits, which are not documented.

CodegenTest.TestCodeCache - fails on my two cpu host, t2.large
increasing the cache size resolves this failure. The test is
depending on:

1) key skew in the SharedLRUCache to obtain hits
2) the fact that capacity of SharedLRUCache is higher than the capacity
configured if the configured capacity is not divisible by the the number
of CPUs.

For example, the capacity is set here:

  FLAGS_codegen_cache_capacity = 10;

However, if the capacity is not perfectly divisible by the number of CPUs,
actual capacity is slightly higher.

CPU 2 => Capacity 10, 5/shard
CPU 4 => Capacity 12, 3/shard
CPU 8 => Capacity 16, 2/shard

Due to this calculation:

const size_t per_shard = (capacity + (num_shards - 1)) / num_shards;

Additionally, the test depends on key skew. For example, I added some temporary
logging which logged each insert. Let's look at inserts into shard 0. Under the
4 CPU case, where each shard has a capacity of 3, shard 3 only sees three 
inserts
in pass 0 resulting in hits on the next pass:

pass: 0
Insert: hash = 460595995, shard = 0
Insert: hash = 339190469, shard = 0
Insert: hash = 326003543, shard = 0
pass: 1

Under the two CPU case, both shard's see more than 5 inserts, causing no cache 
hits.

pass: 0
Insert: hash = 1886151623, shard = 0
Insert: hash = 1395239506, shard = 0
Insert: hash = 1931154674, shard = 0
Insert: hash = 460595995, shard = 0
Insert: hash = 1440596256, shard = 0
Insert: hash = 1870227699, shard = 0
Insert: hash = 1163308785, shard = 0
Insert: hash = 1980547462, shard = 0
Insert: hash = 1106104592, shard = 0
Insert: hash = 1702846352, shard = 0
Insert: hash = 1230845174, shard = 0
Insert: hash = 1903296752, shard = 0
Insert: hash = 1395526688, shard = 0
Insert: hash = 339190469, shard = 0
Insert: hash = 1540160781, shard = 0
Insert: hash = 1377131543, shard = 0
Insert: hash = 2125989246, shard = 0
Insert: hash = 326003543, shard = 0
pass: 1
Insert: hash = 1886151623, shard = 0
Insert: hash = 1395239506, shard = 0
Insert: hash = 1931154674, shard = 0
Insert: hash = 460595995, shard = 0
Insert: hash = 1440596256, shard = 0
Insert: hash = 1870227699, shard = 0
Insert: hash = 1163308785, shard = 0
Insert: hash = 1980547462, shard = 0
Insert: hash = 1106104592, shard = 0
Insert: hash = 1702846352, shard = 0
Insert: hash = 1230845174, shard = 0
Insert: hash = 1903296752, shard = 0
Insert: hash = 1395526688, shard = 0
Insert: hash = 339190469, shard = 0
Insert: hash = 1540160781, shard = 0
Insert: hash = 1377131543, shard = 0
Insert: hash = 2125989246, shard = 0
Insert: hash = 326003543, shard = 0

AFAICT increasing the capacity of the cache doesn't impact correctness.

Change-Id: I81b70f63923078d449f6541a61b292517e49877d
---
M docs/installation.adoc
M src/kudu/codegen/codegen-test.cc
M src/kudu/gutil/sysinfo.cc
3 files changed, 10 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I81b70f63923078d449f6541a61b292517e49877d
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Brock Noland 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Brock Noland 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley

[kudu-CR] [tests] fix tests - two cpus and fresh install

2016-09-24 Thread Brock Noland (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [tests] fix tests - two cpus and fresh install
..

[tests] fix tests - two cpus and fresh install

Various tests fail due to lack of lsof and low resource
limits, which are not documented.

CodegenTest.TestCodeCache - fails on my two cpu host, t2.large
increasing the cache size resolves this failure. The test is
depending on:

1) key skew in the SharedLRUCache to obtain hits
2) the fact that capacity of SharedLRUCache is higher than the capacity
configured if the configured capacity is not divisible by the the number
of CPUs.

For example, the capacity is set here:

  FLAGS_codegen_cache_capacity = 10;

However, if the capacity is not perfectly divisible by the number of CPUs,
actual capacity is slightly higher.

CPU 2 => Capacity 10, 5/shard
CPU 4 => Capacity 12, 3/shard
CPU 8 => Capacity 16, 2/shard

Due to this calculation:

const size_t per_shard = (capacity + (num_shards - 1)) / num_shards;

Additionally, the test depends on key skew. For example, I added some temporary
logging which logged each insert. Let's look at inserts into shard 0. Under the
4 CPU case, where each shard has a capacity of 3, shard 3 only sees three 
inserts
in pass 0 resulting in hits on the next pass:

pass: 0
Insert: hash = 460595995, shard = 0
Insert: hash = 339190469, shard = 0
Insert: hash = 326003543, shard = 0
pass: 1

Under the two CPU case, both shard's see more than 5 inserts, causing no cache 
hits.

pass: 0
Insert: hash = 1886151623, shard = 0
Insert: hash = 1395239506, shard = 0
Insert: hash = 1931154674, shard = 0
Insert: hash = 460595995, shard = 0
Insert: hash = 1440596256, shard = 0
Insert: hash = 1870227699, shard = 0
Insert: hash = 1163308785, shard = 0
Insert: hash = 1980547462, shard = 0
Insert: hash = 1106104592, shard = 0
Insert: hash = 1702846352, shard = 0
Insert: hash = 1230845174, shard = 0
Insert: hash = 1903296752, shard = 0
Insert: hash = 1395526688, shard = 0
Insert: hash = 339190469, shard = 0
Insert: hash = 1540160781, shard = 0
Insert: hash = 1377131543, shard = 0
Insert: hash = 2125989246, shard = 0
Insert: hash = 326003543, shard = 0
pass: 1
Insert: hash = 1886151623, shard = 0
Insert: hash = 1395239506, shard = 0
Insert: hash = 1931154674, shard = 0
Insert: hash = 460595995, shard = 0
Insert: hash = 1440596256, shard = 0
Insert: hash = 1870227699, shard = 0
Insert: hash = 1163308785, shard = 0
Insert: hash = 1980547462, shard = 0
Insert: hash = 1106104592, shard = 0
Insert: hash = 1702846352, shard = 0
Insert: hash = 1230845174, shard = 0
Insert: hash = 1903296752, shard = 0
Insert: hash = 1395526688, shard = 0
Insert: hash = 339190469, shard = 0
Insert: hash = 1540160781, shard = 0
Insert: hash = 1377131543, shard = 0
Insert: hash = 2125989246, shard = 0
Insert: hash = 326003543, shard = 0

AFAICT increasing the capacity of the cache doesn't impact correctness.

Change-Id: I81b70f63923078d449f6541a61b292517e49877d
---
M docs/installation.adoc
M src/kudu/codegen/codegen-test.cc
M src/kudu/gutil/sysinfo.cc
3 files changed, 10 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I81b70f63923078d449f6541a61b292517e49877d
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Brock Noland 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Brock Noland 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley

[kudu-CR] cache: fix behavior on single-CPU systems

2016-09-24 Thread Todd Lipcon (Code Review)

Hello Marcel Kornacker, Adar Dembo,

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

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

to review the following change.

Change subject: cache: fix behavior on single-CPU systems
..

cache: fix behavior on single-CPU systems

On a system with only a single CPU, shard_bits_ would be set to 0. This
would then result in calculating 'hash >> (32 - 0)' which is undefined
behavior. With optimizations, this would turn into a no-op, and we'd end
up using the whole hash as the shard index, instead of 0, causing a
crash.

The fix is to use masking instead of shifting to determine the shard.
This means we'll now use the low bits instead of high bits of the hash
value, but we assume that the hash has good distribution and there is no
particular downside to using low bits instead of high ones.

Tested by manually making NumCPUs return 1 and running cache-test.

Change-Id: I7809e5697df657a589b2ceae5c6d4edbf161b52a
---
M src/kudu/util/cache.cc
1 file changed, 13 insertions(+), 11 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7809e5697df657a589b2ceae5c6d4edbf161b52a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Marcel Kornacker

[kudu-CR] [java] KUDU-1563. Add support for INSERT IGNORE

2016-09-24 Thread Brock Noland (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [java] KUDU-1563. Add support for INSERT IGNORE
..

[java] KUDU-1563. Add support for INSERT IGNORE

Implements java support for the `INSERT IGNORE' operation

Change-Id: Ib0cc4a533dfb01a883d347c9795c165aa8efa3fd
---
A java/kudu-client/src/main/java/org/apache/kudu/client/InsertIgnore.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java
4 files changed, 87 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib0cc4a533dfb01a883d347c9795c165aa8efa3fd
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Brock Noland 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1563. Add support for INSERT IGNORE

2016-09-24 Thread Brock Noland (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1563. Add support for INSERT IGNORE
..

KUDU-1563. Add support for INSERT IGNORE

Add's `INSERT IGNORE' operation which behaves like a
normal `INSERT' except in the case when a duplicate
row error would be raised by the primary key having been
previously inserted.

Follows upsert backend/c++ patch 56c431585ed7ad07ef.

Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/write_op.cc
M src/kudu/client/write_op.h
M src/kudu/common/row_operations-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/row_operations.h
M src/kudu/common/wire_protocol.proto
M src/kudu/integration-tests/fuzz-itest.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/row_op.cc
M src/kudu/tablet/row_op.h
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet-test.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_metrics.cc
M src/kudu/tablet/tablet_metrics.h
M src/kudu/tablet/tablet_random_access-test.cc
M src/kudu/tablet/transactions/transaction.cc
M src/kudu/tablet/transactions/transaction.h
M src/kudu/tablet/transactions/write_transaction.cc
24 files changed, 269 insertions(+), 47 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Brock Noland 
Gerrit-Reviewer: Brock Noland 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Document Impala and Spark integration known issues & limitations

2016-09-24 Thread Will Berkeley (Code Review)

Will Berkeley has submitted this change and it was merged.

Change subject: Document Impala and Spark integration known issues & limitations
..


Document Impala and Spark integration known issues & limitations

Change-Id: I993a09a00f5ab0049fec95e967abc1740b44dc8d
Reviewed-on: http://gerrit.cloudera.org:8080/4443
Tested-by: Dan Burkert 
Reviewed-by: Jean-Daniel Cryans 
---
M docs/developing.adoc
M docs/kudu_impala_integration.adoc
2 files changed, 36 insertions(+), 0 deletions(-)

Approvals:
  Dan Burkert: Verified
  Jean-Daniel Cryans: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I993a09a00f5ab0049fec95e967abc1740b44dc8d
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Will Berkeley

[kudu-CR] KUDU-861 Support changing default, storage attributes

2016-09-24 Thread Will Berkeley (Code Review)

Will Berkeley has posted comments on this change.

Change subject: KUDU-861 Support changing default, storage attributes
..


Patch Set 6:

(10 comments)

I'm going to add coverage in alter_table-randomized-test but wanted to post an 
update since it's been sitting for a few days.

http://gerrit.cloudera.org:8080/#/c/4310/6/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
File 
java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java:

Line 147: ByteString defaultByteString = 
ProtobufHelper.objectToByteStringNoType(name, newDefault);
> This seems extremely fishy to me, but I see that you are basically followin
Yeah, but that's not a good excuse for me not to at least try and improve it. 
Ideas on how to do it better? 

My first thought is that there should be some KuduValue interface, with impls 
for each type, and then the responsibility for transforming into bytes for the 
wire is transferred to each KuduValue impl. Would be less fishy to me, and 
seems like adding new types would be easier, but it's prob a bunch of extra 
code.


http://gerrit.cloudera.org:8080/#/c/4310/6/src/kudu/common/common.proto
File src/kudu/common/common.proto:

PS6, Line 92: required
> best practice is to make all protobuf fields optional.
Done


http://gerrit.cloudera.org:8080/#/c/4310/6/src/kudu/common/schema.cc
File src/kudu/common/schema.cc:

Line 73:   if (read_default_ == NULL) {
> I think the if/else here can be removed, and replaced with only the else cl
Done


Line 82:   if (read_default_ == NULL) {
> same here.
Done


Line 96:   if (write_default_ == NULL) {
> and here
Done


Line 105:   if (write_default_ == NULL) {
> and here
Done


PS6, Line 570: it_names = col_names_.find(col_delta.new_name)) != 
col_names_.end()
> I think this can be simplified using ContainsKey from map-util.h
Done


Line 575:   if ((it_names = col_names_.find(col_delta.name)) == 
col_names_.end()) {
> I think it would be more clear what's going on here if you move
Done


PS6, Line 591: LOG(FATAL) << "Should not reach here";
 :   return Status::IllegalState("Unable to alter column");
> Seems like one of these is appropriate, but not both.
Was following a pattern from elsewhere, but it is weird. Left just the fatal.


http://gerrit.cloudera.org:8080/#/c/4310/6/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:

PS6, Line 353: gscoped_ptr
> prefer using unique_ptr here and elsewhere.
Done. Is there a goal to remove gscoped_ptr in favor of unique_ptr throughout 
the codebase?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I457d99ba2188ef6e439df47c0d94f2dc1a62ea6c
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] [tests] fix test which fails with two cpus and document other dependencies

[kudu-CR] [tests] fix tests - two cpus and fresh install

[kudu-CR] cache: fix behavior on single-CPU systems

[kudu-CR] [java] KUDU-1563. Add support for INSERT IGNORE

[kudu-CR] KUDU-1563. Add support for INSERT IGNORE

[kudu-CR] Document Impala and Spark integration known issues & limitations

[kudu-CR] KUDU-861 Support changing default, storage attributes

7 matches

Site Navigation

Mail list logo

Footer information