date:20161117

[kudu-CR] [python] added KuduPartialRow::SetNoCopy{Binary,String}()

2016-11-17 Thread Jordan Birdsell (Code Review)

Jordan Birdsell has posted comments on this change.

Change subject: [python] added KuduPartialRow::SetNoCopy{Binary,String}()
..


Patch Set 2:

I do agree with clarifying the comments either way though

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If73894c3037f1158cf7b9d3aafecd228a0204bbd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [python] added KuduPartialRow::SetNoCopy{Binary,String}()

2016-11-17 Thread Jordan Birdsell (Code Review)

Jordan Birdsell has posted comments on this change.

Change subject: [python] added KuduPartialRow::SetNoCopy{Binary,String}()
..


Patch Set 2:

Ah, i see, so in that case are you thinking we should use the nocopy routine to 
continue as was done before? If so you'll want to also update the PartialRow 
stuff, otherwise this code does nothing.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If73894c3037f1158cf7b9d3aafecd228a0204bbd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1634 (part 2). Fixed bug which fails to ignore tmp consensus log segments

2016-11-17 Thread Maxim Smyatkin (Code Review)

Maxim Smyatkin has posted comments on this change.

Change subject: KUDU-1634 (part 2). Fixed bug which fails to ignore tmp 
consensus log segments
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5122/1/src/kudu/consensus/log_util.cc
File src/kudu/consensus/log_util.cc:

Line 810:   if (fname.find(kTmpInfix) != fname.npos) {
> If I'm understanding the code in Log::CreatePlaceholderSegment() correctly,
Hm, you're right, the .tmp.newsegmentXX files won't pass the previous check.
But whether there may be any different tmp files or not, I'm not sure. Maybe 
the whole check is really unnecessary. Will check it later.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9cf25fbe6ecfc28aed9443cdfb6fb56100394c73
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin 
Gerrit-HasComments: Yes

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

2016-11-17 Thread Alexey Serbin (Code Review)

Hello Dinesh Bhat, Kudu Jenkins,

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

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

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

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
..

[Timestamp] use 'operator<' instead of ComesBefore

Added more syntactic sugar, now for the kudu::Timestamp class:
use operator{<, <=, >, <=, ==} instead of Timestamp::ComesBefore()
and Timestamp::CompareTo(), where possible.
Besides, this patch contains a minor clean-up on the of header files
and forward declarations.

This patch does not contain any functional changes.

Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
---
M src/kudu/common/timestamp.cc
M src/kudu/common/timestamp.h
M src/kudu/server/hybrid_clock.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_stats.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tserver/tablet_service.cc
9 files changed, 75 insertions(+), 54 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1679 Propagate timestamps for scans

2016-11-17 Thread Alexey Serbin (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1679 Propagate timestamps for scans
..

KUDU-1679 Propagate timestamps for scans

Added the 'propagated_timestamp' field into the ScanResponsePB message.
It's always set by the tablet server which processed the incoming
NewScanRequestPB message successfully.

Also, added a unit test to verify the presence of the field in tablet
server response messages.

Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
---
M src/kudu/client/client-test.cc
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
8 files changed, 203 insertions(+), 56 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Todd Lipcon (Code Review)

Todd Lipcon has submitted this change and it was merged.

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..


KUDU-1749. Allow callers to disable SASL initialization

The Kudu client makes use of SASL, as do other applications such as
Impala that use the client. This means that both Kudu and those
applications are expected to initialize SASL.

The sasl_*_init() functions themselves are safe to call multiple times
(they have no effect). However, sasl_set_mutex() is not safe to call
multiple times, nor is it safe to call after another library has already
begun using the SASL libraries.

IMPALA-4479 describes a crash in SaslMutexUnlock() which is likely
caused by this issue: Impala has initialized the SASL library with no
call to sasl_set_mutex(), and then during Kudu initialization we set the
mutex functions to our own. Then when we call sasl_dispose(), SASL tries
to acquire an internal mutex which was previously allocated to a garbage
value 0x1 by the built-in "fake" mutex implementation. This crashes when
we attempt to access it using our real mutex implementation.

The fix has a few parts:
- we have a new API which allows embedders to disable initialization of
  SASL.
- we try to auto-detect the case in which the client has fully
  initialized SASL including providing a mutex implementation. In that
  case, we skip initialization but log a warning that the caller should
  disable it explicitly.
- if we detect that it's initialized but no mutex implementation is
  provided, we'll log a warning on first usage of SASL.
- if they disable initialization but didn't initialize SASL on their
  own, we'll return an error on first usage.

The one sketchy thing about this patch is that the SASL API doesn't give
a 100% public way to determine its initialization state. However, it
does export a symbol which we can import that provides the necessary
information. This symbol is marked as an API, so we expect it to be
stable enough for usage, at least across the range of supported
operating systems.

Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Reviewed-on: http://gerrit.cloudera.org:8080/5120
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_rpc-test.cc
5 files changed, 212 insertions(+), 10 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
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: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] [integration tests] added scan consistency test

2016-11-17 Thread Alexey Serbin (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [integration tests] added scan consistency test
..

[integration tests] added scan consistency test

This is a small test to expose inconsistent reads when no timestamp
is propagated by the client to tablet servers.

This is in the context of KUDU-1679 Propagate timestamps for scans.

Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/consistency-itest.cc
4 files changed, 401 insertions(+), 0 deletions(-)


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

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

[kudu-CR] [integration tests] added scan consistency test

2016-11-17 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: [integration tests] added scan consistency test
..


Patch Set 2:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/5084/2/src/kudu/client/client-test-util.cc
File src/kudu/client/client-test-util.cc:

Line 21: 
> extra line and include order
This is intentional to conform with our C++ style guide: 
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes


Line 78: row_count_status = scanner->Open();
> if scanner->NextBatch() on LOC 89 timesout won't you Open() a scanner that'
Good catch!  Yes, that I will fix.


http://gerrit.cloudera.org:8080/#/c/5084/2/src/kudu/client/client-test-util.h
File src/kudu/client/client-test-util.h:

PS2, Line 55: CountTableRows
> Maybe call this CountTableRowsWithRetries or something? I would also not be
Yes, I think it's worth updating the name.

Good -- that's another chance for small re-factoring.  I will think of doing 
that in a separate changelist along with putting some methods introduced here 
for re-use by other tests.


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

Line 59: using kudu::master::TableInfo;
> warning: using decl 'TableInfo' is unused [misc-unused-using-decls]
Done


Line 60: using kudu::master::TabletInfo;
> warning: using decl 'TabletInfo' is unused [misc-unused-using-decls]
Done


Line 62: using kudu::tablet::Tablet;
> warning: using decl 'Tablet' is unused [misc-unused-using-decls]
Done


Line 68: using std::thread;
> warning: using decl 'thread' is unused [misc-unused-using-decls]
Done


PS2, Line 90: FLAGS_heartbeat_interval_ms = 10;
> why?
I copied this from client-test.  This is for faster tests: if something goes 
wrong, with this faster setting it reports about an error faster, and overall 
about  700ms per test.

I'll add a comment.


Line 116:  shared_ptr* table) {
> warning: parameter 'table' is unused [misc-unused-parameters]
Done


PS2, Line 130:   unique_ptr BuildTestRow(KuduTable* table, int 
index) {
 : unique_ptr insert(table->NewInsert());
 : KuduPartialRow* row = insert->mutable_row();
 : CHECK_OK(row->SetInt32(0, index));
 : CHECK_OK(row->SetInt32(1, index * 2));
 : CHECK_OK(row->SetStringCopy(2, Slice(StringPrintf("hello 
%d", index;
 : return insert;
 :   }
 : 
 :   // Inserts given number of tests rows into the default test 
table
 :   // in the context of the specified session.
 :   Status InsertTestRows(KuduClient* client, KuduTable* table,
 : int num_rows, int first_row = 0) {
 : shared_ptr session = client->NewSession();
 : 
RETURN_NOT_OK(session->SetFlushMode(KuduSession::AUTO_FLUSH_BACKGROUND));
 : session->SetTimeoutMillis(6);
 : for (int i = first_row; i < num_rows + first_row; ++i) {
 :   unique_ptr insert(BuildTestRow(table, i));
 :   RETURN_NOT_OK(session->Apply(insert.release()));
 : }
 : RETURN_NOT_OK(session->Flush());
 : return Status::OK();
 :   }
 : 
 :   Status GetRowCount(KuduTable* table, KuduScanner::ReadMode 
read_mode,
 :  uint64_t ts, size_t* row_count) {
 : KuduScanner scanner(table);
 : RETURN_NOT_OK(scanner.SetReadMode(read_mode));
 : if (read_mode == KuduScanner::READ_AT_SNAPSHOT && ts != 0) {
 :   RETURN_NOT_OK(scanner.SetSnapshotRaw(ts + 1));
 : }
 : RETURN_NOT_OK(CountTableRows(, row_count));
 : return Status::OK();
 :   }
 : 
 :   Status GetTabletIdForKeyValue(int32_t key_value_begin,
 : int32_t key_value_end,
 : const string& table_name,
 : vector* tablet_ids) {
 : if (!tablet_ids) {
 :   return Status::InvalidArgument("null output container");
 : }
 : tablet_ids->clear();
 : 
 : // Find the tablet for the first range (i.e. for the rows to 
be inserted).
 : unique_ptr split_row_start(schema_.NewRow());
 : RETURN_NOT_OK(split_row_start->SetInt32(0, key_value_begin));
 : string partition_key_start;
 : 
RETURN_NOT_OK(split_row_start->EncodeRowKey(_key_start));
 : 
 : unique_ptr split_row_end(schema_.NewRow());
 :

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

2016-11-17 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
..


Patch Set 3:

(4 comments)

lgtm, but please remove the CompactionInputRow stuff

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

PS3, Line 474: 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
same


PS3, Line 350: if (smallest < state->next()) {
same


PS3, Line 565: namespace {
 : void AdvanceToLastInList(const Mutation** m) {
 :   const Mutation* next;
 :   while ((next = (*m)->acquire_next()) != nullptr) {
 : *m = next;
 :   }
 : }
 : } // anonymous namespace
 : 
 : // Compare the mutations of two duplicated rows.
 : // Return 'true' if latest_version(left) < latest_version(right)
 : bool operator<(const CompactionInputRow& lhs, const 
CompactionInputRow& rhs) {
 :   const Mutation* left_latest = lhs.redo_head;
 :   const Mutation* right_latest = rhs.redo_head;
 :   if (right_latest == nullptr) {
 : DCHECK(left_latest != nullptr);
 : return true;
 :   }
 :   if (left_latest == nullptr) {
 : // left must still be alive
 : DCHECK(right_latest != nullptr);
 : return false;
 :   }
 : 
 :   // Duplicated rows have disjoint histories, we don't need to 
get the latest
 :   // mutation, the first one should be enough for the sake of 
determining the most recent
 :   // row, but in debug mode do get the latest to make sure one 
of the rows is a ghost.
 :   const bool ret = left_latest->timestamp() < 
right_latest->timestamp();
 : #ifndef NDEBUG
 :   AdvanceToLastInList(_latest);
 :   AdvanceToLastInList(_latest);
 :   if (left_latest->timestamp() != right_latest->timestamp()) {
 : // If in fact both rows were deleted at the same time, this 
is OK -- we could
 : // have a case like TestRandomAccess.TestFuzz3, in which a 
single batch
 : // DELETED from the DRS, INSERTed into MRS, and DELETED from 
MRS. In that case,
 : // the timestamp of the last REDO will be the same and we 
can pick whichever
 : // we like.
 : const bool debug_ret = left_latest->timestamp() < 
right_latest->timestamp();
 : CHECK_EQ(ret, debug_ret);
 :   }
 : #endif
 :   return ret;
 : }
same


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

PS3, Line 167: bool operator<(const CompactionInputRow& lhs, const 
CompactionInputRow& rhs);
this seems out of place in this patch (the commit message only mentions 
Timestamp) and actually goes against the reinsert stuff I have in-flight where 
I completely change the comparison between two CompactionInputRows, mind 
removing this from this patch?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1750: Drop range partition is not working as expected

2016-11-17 Thread Dan Burkert (Code Review)

Dan Burkert has submitted this change and it was merged.

Change subject: KUDU-1750: Drop range partition is not working as expected
..


KUDU-1750: Drop range partition is not working as expected

Change-Id: I82c11fbc767f81ab3fe8f486d76a45e3ca3f5b9a
Reviewed-on: http://gerrit.cloudera.org:8080/5133
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 196 insertions(+), 2 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I82c11fbc767f81ab3fe8f486d76a45e3ca3f5b9a
Gerrit-PatchSet: 3
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

[kudu-CR] KUDU-1750: Drop range partition is not working as expected

2016-11-17 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: KUDU-1750: Drop range partition is not working as expected
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I82c11fbc767f81ab3fe8f486d76a45e3ca3f5b9a
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: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1750: Drop range partition is not working as expected

2016-11-17 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change.

Change subject: KUDU-1750: Drop range partition is not working as expected
..


Patch Set 1:

(1 comment)

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

PS1, Line 1308:  KUDU-1750 Regression cases
> If not there, can you also add a test case with dropping unbounded range pa
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I82c11fbc767f81ab3fe8f486d76a45e3ca3f5b9a
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: Yes

[kudu-CR] KUDU-1750: Drop range partition is not working as expected

2016-11-17 Thread Dan Burkert (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1750: Drop range partition is not working as expected
..

KUDU-1750: Drop range partition is not working as expected

Change-Id: I82c11fbc767f81ab3fe8f486d76a45e3ca3f5b9a
---
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 196 insertions(+), 2 deletions(-)


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

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

[kudu-CR] KUDU-1750: Drop range partition is not working as expected

2016-11-17 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change.

Change subject: KUDU-1750: Drop range partition is not working as expected
..


Patch Set 1:

(1 comment)

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

PS1, Line 1308:  KUDU-1750 Regression cases
If not there, can you also add a test case with dropping unbounded range 
partitions? e.g. add (, 100]. drop (, 200] or something along these lines.


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

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

[kudu-CR] KUDU-1750: Drop range partition is not working as expected

2016-11-17 Thread 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/5133

to review the following change.

Change subject: KUDU-1750: Drop range partition is not working as expected
..

KUDU-1750: Drop range partition is not working as expected

Change-Id: I82c11fbc767f81ab3fe8f486d76a45e3ca3f5b9a
---
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 64 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I82c11fbc767f81ab3fe8f486d76a45e3ca3f5b9a
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] KUDU-1679 Propagate timestamps for scans

2016-11-17 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1679 Propagate timestamps for scans
..


Patch Set 5:

> Could you put this patch after the consistency itest patch in the
 > git history and enable that test here?

Good idea.  Sure, will do.

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

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

[kudu-CR] [python] added KuduPartialRow::SetNoCopy{Binary,String}()

2016-11-17 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: [python] added KuduPartialRow::SetNoCopy{Binary,String}()
..


Patch Set 2:

> didnt finish my sentence above...given the way python handles the
 > semantics for PartialRows, im not sure if theres any way to deal
 > with the NoCopy setters, also not 100% sure what effect python GC
 > would have on using a NoCopy setter.

Well, supposedly it does the same thing as it did for old semantics of 
SetString/SetBinary -- prior to 48766a4ce17d422ced9a6ec78c9a9969ac44d8c9 they 
were not copying data.  Basically, older Set{String,Binary} behaved like 
current Set{String,Binary}NoCopy.

If you think it requires additional clarification -- sure, let's do that.  I 
just assumed that since it worked for Set{String,Binary} before, it's going to 
work for Set{String,Binary}NoCopy now.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If73894c3037f1158cf7b9d3aafecd228a0204bbd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [python] added KuduPartialRow::SetNoCopy{Binary,String}()

2016-11-17 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: [python] added KuduPartialRow::SetNoCopy{Binary,String}()
..


Patch Set 2:

> Was this more for parity's sake or did you need this exposed? The
 > libkudu_client module really just exposes the external signatures
 > to python so if you want this to be exposed in the client it would
 > need to be implemented in the PartialRow class in client.pyx. 
 > However, given the way python handles the semantics for PartialRows
 > (schema.new_row({'foo': 'bar'} or alternatively row['foo'] = 'bar'

Yes, that was more about parity and also updating the stale comment about the 
SetString/SetBinary -- those changed their sematics.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If73894c3037f1158cf7b9d3aafecd228a0204bbd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5120/6/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

PS6, Line 28: #include 
nit: does it make sense to include only for non-macOS builds?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5120/6/src/kudu/rpc/sasl_rpc-test.cc
File src/kudu/rpc/sasl_rpc-test.cc:

PS6, Line 570: // Test a client which inits SASL itself but doesn't remember to 
disable Kudu's
 : // SASL initialization
nit: is it worth mentioning in the comment that the client does not do proper 
mutex initialization?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

2016-11-17 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change.

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


Patch Set 12:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/4310/12/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:

PS12, Line 133: `newDefault` must not be null or
  :* else throws {@link IllegalArgumentException}.
Does Kudu allow a nullable column to not have a default value (e.g. error on 
omission)?


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

Line 256: } else if (value instanceof byte[]) {
PartialRow allows setting a binary column with a ByteBuffer, so that should 
probably work here as well.


http://gerrit.cloudera.org:8080/#/c/4310/12/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduTable.java:

PS12, Line 88: pizza
I think this is going to fail when you rebase against master.  The debug format 
of string columns was recently changed to include quotes around the value.


Line 135:   syncClient.deleteTable(tableName);
We just fixed this, so you can remove this try/finally.


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

Line 60: if (col_delta.has_default && col_delta.default_value.size() < 
type_info()->size()) {
Should this be a != instead of the inequality?


PS12, Line 71: NULL
prefer using nullptr instead of NULL


Line 80:   if (write_default_ == NULL) {
I commented on this previously but I think it got lost in the shuffle - Can 
these if/else trees be collapsed to just the else branch?  It seems like Reset 
should work even if the current value is nullptr.


Line 494:   unordered_set::const_iterator it_names;
move this down to above the second if statement, and initialize it with the 
declaration.  Goal here is to remove all the initialization-in-if-conditions, I 
find those difficult.


Line 497:   if ((it_names = col_names_.find(new_name)) != col_names_.end()) {
Don't use a local variable here, since it's not referenced again.


PS12, Line 547: = col_names_.find(col_delta.name)
move this initialization to the line above.


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

Line 128:   std::string new_name;
I think it would be cleaner if all of these fields were wrapped in a 
boost::optional instead of having the associated boolean.


PS12, Line 281: return
returns


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

PS12, Line 266:   
indentation


-- 
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: 12
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] KUDU-1679 Propagate timestamps for scans

2016-11-17 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1679 Propagate timestamps for scans
..


Patch Set 5:

Could you put this patch after the consistency itest patch in the git history 
and enable that test here?

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

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

[kudu-CR] KUDU-1679 Propagate timestamps for scans

2016-11-17 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1679 Propagate timestamps for scans
..


Patch Set 5:

(1 comment)

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

PS4, Line 166: uint64_t snapshot_timestamp_;
> For C++ mapping the PB field is uint64:
good point. dunno why I thought it mapped to int64


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

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

[kudu-CR] [integration tests] added scan consistency test

2016-11-17 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: [integration tests] added scan consistency test
..


Patch Set 2:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/5084/2/src/kudu/client/client-test-util.cc
File src/kudu/client/client-test-util.cc:

Line 21: 
extra line and include order


Line 78: row_count_status = scanner->Open();
if scanner->NextBatch() on LOC 89 timesout won't you Open() a scanner that's 
already open here?


http://gerrit.cloudera.org:8080/#/c/5084/2/src/kudu/client/client-test-util.h
File src/kudu/client/client-test-util.h:

PS2, Line 55: CountTableRows
Maybe call this CountTableRowsWithRetries or something? I would also not be 
averse to making this one the only version.


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

PS2, Line 90: FLAGS_heartbeat_interval_ms = 10;
why?


PS2, Line 130:   unique_ptr BuildTestRow(KuduTable* table, int 
index) {
 : unique_ptr insert(table->NewInsert());
 : KuduPartialRow* row = insert->mutable_row();
 : CHECK_OK(row->SetInt32(0, index));
 : CHECK_OK(row->SetInt32(1, index * 2));
 : CHECK_OK(row->SetStringCopy(2, Slice(StringPrintf("hello 
%d", index;
 : return insert;
 :   }
 : 
 :   // Inserts given number of tests rows into the default test 
table
 :   // in the context of the specified session.
 :   Status InsertTestRows(KuduClient* client, KuduTable* table,
 : int num_rows, int first_row = 0) {
 : shared_ptr session = client->NewSession();
 : 
RETURN_NOT_OK(session->SetFlushMode(KuduSession::AUTO_FLUSH_BACKGROUND));
 : session->SetTimeoutMillis(6);
 : for (int i = first_row; i < num_rows + first_row; ++i) {
 :   unique_ptr insert(BuildTestRow(table, i));
 :   RETURN_NOT_OK(session->Apply(insert.release()));
 : }
 : RETURN_NOT_OK(session->Flush());
 : return Status::OK();
 :   }
 : 
 :   Status GetRowCount(KuduTable* table, KuduScanner::ReadMode 
read_mode,
 :  uint64_t ts, size_t* row_count) {
 : KuduScanner scanner(table);
 : RETURN_NOT_OK(scanner.SetReadMode(read_mode));
 : if (read_mode == KuduScanner::READ_AT_SNAPSHOT && ts != 0) {
 :   RETURN_NOT_OK(scanner.SetSnapshotRaw(ts + 1));
 : }
 : RETURN_NOT_OK(CountTableRows(, row_count));
 : return Status::OK();
 :   }
 : 
 :   Status GetTabletIdForKeyValue(int32_t key_value_begin,
 : int32_t key_value_end,
 : const string& table_name,
 : vector* tablet_ids) {
 : if (!tablet_ids) {
 :   return Status::InvalidArgument("null output container");
 : }
 : tablet_ids->clear();
 : 
 : // Find the tablet for the first range (i.e. for the rows to 
be inserted).
 : unique_ptr split_row_start(schema_.NewRow());
 : RETURN_NOT_OK(split_row_start->SetInt32(0, key_value_begin));
 : string partition_key_start;
 : 
RETURN_NOT_OK(split_row_start->EncodeRowKey(_key_start));
 : 
 : unique_ptr split_row_end(schema_.NewRow());
 : RETURN_NOT_OK(split_row_end->SetInt32(0, key_value_end));
 : string partition_key_end;
 : 
RETURN_NOT_OK(split_row_end->EncodeRowKey(_key_end));
 : 
 : GetTableLocationsRequestPB req;
 : req.mutable_table()->set_table_name(table_name);
 : req.set_partition_key_start(partition_key_start);
 : req.set_partition_key_end(partition_key_end);
 : master::CatalogManager* catalog =
 : cluster_->mini_master()->master()->catalog_manager();
 : GetTableLocationsResponsePB resp;
 : CatalogManager::ScopedLeaderSharedLock l(catalog);
 : RETURN_NOT_OK(l.first_failed_status());
 : RETURN_NOT_OK(catalog->GetTableLocations(, ));
 : for (size_t i = 0; i < resp.tablet_locations_size(); ++i) {
 :   
tablet_ids->emplace_back(resp.tablet_locations(i).tablet_id());
 : }
 : 
 : return Status::OK();
 :   }
any way to re-use some stuff from other tests? Maybe add

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..


Patch Set 6: Code-Review+2

Looks good to me, though Alexey/Todd may want to have another look.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change.

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

PS3, Line 218:  
> extra space
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change.

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/client/client.h
File src/kudu/client/client.h:

PS3, Line 132: SASL
> I think changing just this one instance would be enough to precisely indica
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change.

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/rpc/sasl_rpc-test.cc
File src/kudu/rpc/sasl_rpc-test.cc:

PS3, Line 511:   executable_path,
 :   "test",
 :   filter_flag,
 :   "--is_test_child"},
> Nit: indentation is strangely unaligned here.
Done


http://gerrit.cloudera.org:8080/#/c/5120/4/src/kudu/rpc/sasl_rpc-test.cc
File src/kudu/rpc/sasl_rpc-test.cc:

PS4, Line 561: rpc::internal::SaslSetMutex();
> Does it make sense to add a test where this is skipped by the client?  I.e.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Dan Burkert (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..

KUDU-1749. Allow callers to disable SASL initialization

The Kudu client makes use of SASL, as do other applications such as
Impala that use the client. This means that both Kudu and those
applications are expected to initialize SASL.

The sasl_*_init() functions themselves are safe to call multiple times
(they have no effect). However, sasl_set_mutex() is not safe to call
multiple times, nor is it safe to call after another library has already
begun using the SASL libraries.

IMPALA-4479 describes a crash in SaslMutexUnlock() which is likely
caused by this issue: Impala has initialized the SASL library with no
call to sasl_set_mutex(), and then during Kudu initialization we set the
mutex functions to our own. Then when we call sasl_dispose(), SASL tries
to acquire an internal mutex which was previously allocated to a garbage
value 0x1 by the built-in "fake" mutex implementation. This crashes when
we attempt to access it using our real mutex implementation.

The fix has a few parts:
- we have a new API which allows embedders to disable initialization of
  SASL.
- we try to auto-detect the case in which the client has fully
  initialized SASL including providing a mutex implementation. In that
  case, we skip initialization but log a warning that the caller should
  disable it explicitly.
- if we detect that it's initialized but no mutex implementation is
  provided, we'll log a warning on first usage of SASL.
- if they disable initialization but didn't initialize SASL on their
  own, we'll return an error on first usage.

The one sketchy thing about this patch is that the SASL API doesn't give
a 100% public way to determine its initialization state. However, it
does export a symbol which we can import that provides the necessary
information. This symbol is marked as an API, so we expect it to be
stable enough for usage, at least across the range of supported
operating systems.

Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_rpc-test.cc
5 files changed, 212 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change.

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/client/client.h
File src/kudu/client/client.h:

PS3, Line 132: SASL
> Only if the client is also using the Cyrus SASL library, right? In theory t
Yes in theory.  In practice the overlap between people using custom sasl 
libraries and kudu is probably non-existent.

Would you like me to change SASL to Cyrus SASL everywhere it's used, or 
something else to make it clearer?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1747 addColumn API is wrong wrt nullability and default values

2016-11-17 Thread Dimitris Tsirogiannis (Code Review)

Dimitris Tsirogiannis has posted comments on this change.

Change subject: KUDU-1747 addColumn API is wrong wrt nullability and default 
values
..


Patch Set 1: Code-Review+1

(1 comment)

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

Line 129: }
I would also expand the test to make sure that I can insert a row that has a 
null in the addNullableDef column and when I read it back I get the null value 
and not some default.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0706839a3dbe21532afd524e2d3fc020d31a9973
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Dan Burkert (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..

KUDU-1749. Allow callers to disable SASL initialization

The Kudu client makes use of SASL, as do other applications such as
Impala that use the client. This means that both Kudu and those
applications are expected to initialize SASL.

The sasl_*_init() functions themselves are safe to call multiple times
(they have no effect). However, sasl_set_mutex() is not safe to call
multiple times, nor is it safe to call after another library has already
begun using the SASL libraries.

IMPALA-4479 describes a crash in SaslMutexUnlock() which is likely
caused by this issue: Impala has initialized the SASL library with no
call to sasl_set_mutex(), and then during Kudu initialization we set the
mutex functions to our own. Then when we call sasl_dispose(), SASL tries
to acquire an internal mutex which was previously allocated to a garbage
value 0x1 by the built-in "fake" mutex implementation. This crashes when
we attempt to access it using our real mutex implementation.

The fix has a few parts:
- we have a new API which allows embedders to disable initialization of
  SASL.
- we try to auto-detect the case in which the client has fully
  initialized SASL including providing a mutex implementation. In that
  case, we skip initialization but log a warning that the caller should
  disable it explicitly.
- if we detect that it's initialized but no mutex implementation is
  provided, we'll log a warning on first usage of SASL.
- if they disable initialization but didn't initialize SASL on their
  own, we'll return an error on first usage.

The one sketchy thing about this patch is that the SASL API doesn't give
a 100% public way to determine its initialization state. However, it
does export a symbol which we can import that provides the necessary
information. This symbol is marked as an API, so we expect it to be
stable enough for usage, at least across the range of supported
operating systems.

Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_rpc-test.cc
5 files changed, 200 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
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: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1722 - Range Partition Failure on UNIXTIME MICROS

2016-11-17 Thread Jordan Birdsell (Code Review)

Jordan Birdsell has submitted this change and it was merged.

Change subject: KUDU-1722 - Range Partition Failure on UNIXTIME_MICROS
..


KUDU-1722 - Range Partition Failure on UNIXTIME_MICROS

Errors are returned when attempting to add range partition bounds
for a unixtime_micros value when specifying bound types that are
not the default. This is happening because the logic to convert the
bounds to defaults (inclusive for Lower and exclusive for Upper)
converts the UNIXTIME_MICROS value to an INT64. This patch addresses
this issue and includes a test.

Change-Id: I47d23d184a6b73c5c0cdd04519821c4e7eb53ecb
Reviewed-on: http://gerrit.cloudera.org:8080/5119
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M src/kudu/client/client-test.cc
M src/kudu/common/partition.cc
2 files changed, 39 insertions(+), 3 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I47d23d184a6b73c5c0cdd04519821c4e7eb53ecb
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1722 - Range Partition Failure on UNIXTIME MICROS

2016-11-17 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change.

Change subject: KUDU-1722 - Range Partition Failure on UNIXTIME_MICROS
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47d23d184a6b73c5c0cdd04519821c4e7eb53ecb
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] el6: fix krb5 realm workaround for static builds

2016-11-17 Thread Dan Burkert (Code Review)

Dan Burkert has submitted this change and it was merged.

Change subject: el6: fix krb5 realm workaround for static builds
..


el6: fix krb5 realm workaround for static builds

Here's another attempt at enabling the workaround for the numeric realm
problem on el6 (see ba2ae3de4a7c43ff2f5873e822410e066ea99667 for
background).

The previous attempt didn't succeed for static builds because we were
setting LD_PRELOAD to the non-absolute path of the override library.
This worked OK in debug builds because our binary RUNPATHs included the
build/lib/ directory. Static builds didn't have such a RUNPATH and thus
the LD_PRELOAD was unable to locate the specified shared object.

The two options to fix this were (1) determine and specify an absolute
path, or (2) build this workaround into all of our binaries rather than
try to preload it at runtime.

The issue with #1 is that, when we get to Java-based testing, it would
have been inconvenient to determine the absolute path of the override
library. In addition, it would make test binaries non-relocatable, which
is somewhat of a hassle.

So, this takes approach #2. The build incantations are a bit nasty. In
order to make sure that the symbol shows up everywhere, and shows up
before the krb5 library, and doesn't show up more than once (to avoid
ODR violations), we have to manually add it to the link list for the
the executables that need the override. This includes the tests (and
thus the 'test_main' library) as well as any executables which are run
by the tests ((tserver, master, and 'kudu' tool).

In addition, we need to make sure that the linker doesn't entirely skip
the override binary. I accomplished this using the '--undefined' linker
flag.

I tested this patch using both sasl_rpc-test and
external_mini_cluster-test on an el6 box in both debug (dynamic linked)
and release (static-linked) builds. I also fully built both build types
to ensure there were no linker issues in unrelated tests.

Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Reviewed-on: http://gerrit.cloudera.org:8080/5100
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
Reviewed-by: Dan Burkert 
---
M CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/security/CMakeLists.txt
R src/kudu/security/krb5_realm_override.cc
M src/kudu/tools/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
M src/kudu/util/test_util.cc
11 files changed, 39 insertions(+), 38 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I050d03d58658b650891566b9f955c867b15731e0
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] el6: fix krb5 realm workaround for static builds

2016-11-17 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change.

Change subject: el6: fix krb5 realm workaround for static builds
..


Patch Set 5: Code-Review+2

oops wrong patch

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

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

[kudu-CR] el6: fix krb5 realm workaround for static builds

2016-11-17 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change.

Change subject: el6: fix krb5 realm workaround for static builds
..


Patch Set 5: Code-Review-1

Still broken on OS X - see previous comment with gist.

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

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

[kudu-CR](gh-pages) Fix list rendering of the last weekly update post

2016-11-17 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has submitted this change and it was merged.

Change subject: Fix list rendering of the last weekly update post
..


Fix list rendering of the last weekly update post

The sublist after "Noteworthy features/improvements" is not rendering
properly. Apparently it was missing a new line to be indentified as a
list but adding it made things look weird too (a new line before the
list but none after). This makes the list a "definition list" which
boldens the title and avoids the new line weirdness at the cost of
extra indenting.

Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498
Reviewed-on: http://gerrit.cloudera.org:8080/5131
Reviewed-by: Todd Lipcon 
Tested-by: David Ribeiro Alves 
---
M _posts/2016-11-15-weekly-update.md
1 file changed, 2 insertions(+), 2 deletions(-)

Approvals:
  David Ribeiro Alves: Verified
  Todd Lipcon: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1747 addColumn API is wrong wrt nullability and default values

2016-11-17 Thread Will Berkeley (Code Review)

Will Berkeley has uploaded a new change for review.

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

Change subject: KUDU-1747 addColumn API is wrong wrt nullability and default 
values
..

KUDU-1747 addColumn API is wrong wrt nullability and default values

Previously, the java client permitted adding

- a non-null column with a default
- a nullable column with no default

but did not allow adding

- a nullable column with a default

even though it should be allowed. This patch adds that capability
by adding a 'defaultVal' argument to an overloaded
AlterTableOptions.addNullableColumn. It is backwards-compatible.

Additionally, a test was added to check behavior of the three
addColumn possibilities.

Change-Id: I0706839a3dbe21532afd524e2d3fc020d31a9973
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
2 files changed, 43 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0706839a3dbe21532afd524e2d3fc020d31a9973
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Todd Lipcon (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..

KUDU-1749. Allow callers to disable SASL initialization

The Kudu client makes use of SASL, as do other applications such as
Impala that use the client. This means that both Kudu and those
applications are expected to initialize SASL.

The sasl_*_init() functions themselves are safe to call multiple times
(they have no effect). However, sasl_set_mutex() is not safe to call
multiple times, nor is it safe to call after another library has already
begun using the SASL libraries.

IMPALA-4479 describes a crash in SaslMutexUnlock() which is likely
caused by this issue: Impala has initialized the SASL library with no
call to sasl_set_mutex(), and then during Kudu initialization we set the
mutex functions to our own. Then when we call sasl_dispose(), SASL tries
to acquire an internal mutex which was previously allocated to a garbage
value 0x1 by the built-in "fake" mutex implementation. This crashes when
we attempt to access it using our real mutex implementation.

The fix has a few parts:
- we have a new API which allows embedders to disable initialization of
  SASL.
- we try to auto-detect the case in which the client has fully
  initialized SASL including providing a mutex implementation. In that
  case, we skip initialization but log a warning that the caller should
  disable it explicitly.
- if we detect that it's initialized but no mutex implementation is
  provided, we'll log a warning on first usage of SASL.
- if they disable initialization but didn't initialize SASL on their
  own, we'll return an error on first usage.

The one sketchy thing about this patch is that the SASL API doesn't give
a 100% public way to determine its initialization state. However, it
does export a symbol which we can import that provides the necessary
information. This symbol is marked as an API, so we expect it to be
stable enough for usage, at least across the range of supported
operating systems.

Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_rpc-test.cc
5 files changed, 183 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR](gh-pages) Fix list rendering of the last weekly update post

2016-11-17 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: Fix list rendering of the last weekly update post
..


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR](gh-pages) Fix list rendering of the last weekly update post

2016-11-17 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change.

Change subject: Fix list rendering of the last weekly update post
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] el6: fix krb5 realm workaround for static builds

2016-11-17 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: el6: fix krb5 realm workaround for static builds
..


Patch Set 5: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5100/5/CMakeLists.txt
File CMakeLists.txt:

Line 952: set(KRB5_REALM_OVERRIDE -Wl,--undefined=krb5_realm_override_loaded 
krb5_realm_override)
Nit: indent a bit.


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

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

[kudu-CR] KUDU-1722 - Range Partition Failure on UNIXTIME MICROS

2016-11-17 Thread Jordan Birdsell (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1722 - Range Partition Failure on UNIXTIME_MICROS
..

KUDU-1722 - Range Partition Failure on UNIXTIME_MICROS

Errors are returned when attempting to add range partition bounds
for a unixtime_micros value when specifying bound types that are
not the default. This is happening because the logic to convert the
bounds to defaults (inclusive for Lower and exclusive for Upper)
converts the UNIXTIME_MICROS value to an INT64. This patch addresses
this issue and includes a test.

Change-Id: I47d23d184a6b73c5c0cdd04519821c4e7eb53ecb
---
M src/kudu/client/client-test.cc
M src/kudu/common/partition.cc
2 files changed, 39 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I47d23d184a6b73c5c0cdd04519821c4e7eb53ecb
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] tablet copy: Make the StartTabletCopy() RPC async

2016-11-17 Thread Dinesh Bhat (Code Review)

Dinesh Bhat has posted comments on this change.

Change subject: tablet copy: Make the StartTabletCopy() RPC async
..


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/5045/2/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

Line 320: 
spurious \n ?


PS2, Line 436: success_callback
I am curious why we needed a different callback for success ? One callback can 
have 
if (status == success ) 
  OpenTablet()
else 
  Tombstone()

?


PS2, Line 444: open_tablet_pool_
Interesting, just thinking out loud here about possible outcomes of using the 
same threadpool as that of OpenTablet(): Given that open_tablet_pool_ contains 
as many max threads as there are tablets to open during server bring up, tablet 
copy could contend with OpenTablet() threads if consensus for these tablets 
starts up in the context of OpenTablet(). i.e, leader could get to know about 
follower before OpenTablet thread finishes and starts a tablet copy session 
(because it is tombstoned or whatever reason) and now there is a possibility 
that there are no threads available to start async tablet copy ? Or am I just 
hallucinating ?


PS2, Line 446: TOMBSTONE_NOT_OK
There is nothing to tombstone at this point right ? We could as well leave the 
tablet as it is since it hasn't changed its state yet.


http://gerrit.cloudera.org:8080/#/c/5045/2/src/kudu/tserver/ts_tablet_manager.h
File src/kudu/tserver/ts_tablet_manager.h:

PS2, Line 328: tablet_copy_sessions_
Good idea, we could eventually connect this to metrics or tools to show the 
number of tablets being tablet copied, right ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I95c63f2bfd6762487862efbdba9cb3676112
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [docs] Add krb5-devel to the SLES12 instructions

2016-11-17 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: [docs] Add krb5-devel to the SLES12 instructions
..


Patch Set 1:

(2 comments)

I bet you need the equivalent of krb5-devel in the other distros too. There's 
libkrb5-dev on Ubuntu which provides /usr/include/krb5.h, and 
/usr/include/gssapi as well as a bunch of shared objects.

http://gerrit.cloudera.org:8080/#/c/5130/1/docs/installation.adoc
File docs/installation.adoc:

Line 451: $ sudo zypper install autoconf automake curl cyrus-sasl-devel \
I think Dan resorted this; could you maintain the sort order?


Line 504: sudo zypper install -y autoconf automake curl cyrus-sasl-devel \
Same.


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

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

[kudu-CR] [docs] Add krb5-devel to the SLES12 instructions

2016-11-17 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has posted comments on this change.

Change subject: [docs] Add krb5-devel to the SLES12 instructions
..


Patch Set 1:

(1 comment)

> (2 comments)
 > 
 > I bet you need the equivalent of krb5-devel in the other distros
 > too. There's libkrb5-dev on Ubuntu which provides /usr/include/krb5.h,
 > and /usr/include/gssapi as well as a bunch of shared objects.

I'm just fixing the SLES12 install docs, I didn't look at the other platforms. 
AFAIK they're supposed to be complete (looks at Dan).

http://gerrit.cloudera.org:8080/#/c/5130/1/docs/installation.adoc
File docs/installation.adoc:

Line 451: $ sudo zypper install autoconf automake curl cyrus-sasl-devel \
> I think Dan resorted this; could you maintain the sort order?
Oh I didn't even notice, I was just putting it next to openssl. Thanks!


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

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

[kudu-CR] [docs] Add krb5-devel to the SLES12 instructions

2016-11-17 Thread Jean-Daniel Cryans (Code Review)

Hello Dan Burkert, Todd Lipcon,

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

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

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

Change subject: [docs] Add krb5-devel to the SLES12 instructions
..

[docs] Add krb5-devel to the SLES12 instructions

Change-Id: I9f98443a923edb6687427a5863e969883266bd30
---
M docs/installation.adoc
1 file changed, 2 insertions(+), 2 deletions(-)


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

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

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change.

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

PS3, Line 218:  
extra space


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..


Patch Set 3:

(2 comments)

Mostly just wanted to look at the API changes.

http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/client/client.h
File src/kudu/client/client.h:

PS3, Line 132: SASL
Only if the client is also using the Cyrus SASL library, right? In theory there 
are other SASL implementations for which there'd be no overlap at all?


http://gerrit.cloudera.org:8080/#/c/5120/3/src/kudu/rpc/sasl_rpc-test.cc
File src/kudu/rpc/sasl_rpc-test.cc:

PS3, Line 511:   executable_path,
 :   "test",
 :   filter_flag,
 :   "--is_test_child"},
Nit: indentation is strangely unaligned here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR](gh-pages) Fix list rendering of the last weekly update post

2016-11-17 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has uploaded a new patch set (#2).

Change subject: Fix list rendering of the last weekly update post
..

Fix list rendering of the last weekly update post

The sublist after "Noteworthy features/improvements" is not rendering
properly. Apparently it was missing a new line to be indentified as a
list but adding it made things look weird too (a new line before the
list but none after). This makes the list a "definition list" which
boldens the title and avoids the new line weirdness at the cost of
extra indenting.

Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498
---
M _posts/2016-11-15-weekly-update.md
1 file changed, 2 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: David Ribeiro Alves

[kudu-CR](gh-pages) Fix list rendering of the last weekly update post

2016-11-17 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: Fix list rendering of the last weekly update post
..

Fix list rendering of the last weekly update post

The sublist after "Noteworthy features/improvements" is not rendering
properly. Apparently it was missing a new line to be indentified as a
list but adding it made things look weird too (a new line before the
list but none after). This makes the list a "definition list" which
boldens the title and avoids the new line weirdness at the cost of
extra indenting.

Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498
---
M _posts/2016-11-15-weekly-update.md
1 file changed, 2 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I78abc84ee430c0c73536ebc5691c39c47f836498
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: David Ribeiro Alves

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change.

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..


Patch Set 2:

(2 comments)

Here's a patch to make this work with OS X: 
https://gist.github.com/anonymous/bc2d7fe7769cd50239ce1a27bec7d497

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

PS2, Line 19: Impala has initialized the SASL library with no
: call to sasl_set_mutex(), and then during Kudu initialization we 
set the
: mutex functions to our own.
Has Impala been fixed?  If not, this patch still won't work correctly since it 
will fail when the mutex check is done.


http://gerrit.cloudera.org:8080/#/c/5120/2/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

Line 203: LOG(WARNING) << "SASL was initialized prior to Kudu's 
initialization. Skipping "
I think I would be in favor of making this a hard error.


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

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

[kudu-CR] [docs] Add krb5-devel to the SLES12 instructions

2016-11-17 Thread Jean-Daniel Cryans (Code Review)

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: [docs] Add krb5-devel to the SLES12 instructions
..

[docs] Add krb5-devel to the SLES12 instructions

Change-Id: I9f98443a923edb6687427a5863e969883266bd30
---
M docs/installation.adoc
1 file changed, 2 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9f98443a923edb6687427a5863e969883266bd30
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup

2016-11-17 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change.

Change subject: KUDU-1634. TS and master should delete tmp metadata files on 
startup
..


Patch Set 5:

FYI, Maxim is working on a change to the infix in a follow-on patch so I'm 
going to merge this one as-is.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 5
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] KUDU-1634. TS and master should delete tmp metadata files on startup

2016-11-17 Thread Adar Dembo (Code Review)

Adar Dembo has submitted this change and it was merged.

Change subject: KUDU-1634. TS and master should delete tmp metadata files on 
startup
..


KUDU-1634. TS and master should delete tmp metadata files on startup

Initially it was suggested that TS should delete tmp files from consensus-meta
directory, but these leftovers can appear anywhere where atomic write with
WritePBContainerToPath() is being used. For now, it is consensus metadata,
block manager's data directory, instance metadata, etc. Also, it happens to
Master server as well.
Hence, I've decided to deal with it on FsManager level and scan all root paths
for tmp files. It may slow startup a little bit, but I don't know any way
around it because:
a) it can't be done on background as it can remove the real tmp files
this way;
b) it would be nice to have a separate tmp directory, but it makes rename not
atomic this way (because we can - and usually will - have root/wal dirs on
different drives).

Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Reviewed-on: http://gerrit.cloudera.org:8080/5007
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
3 files changed, 154 insertions(+), 6 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 6
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] [integration tests] added scan consistency test

2016-11-17 Thread Alexey Serbin (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: [integration tests] added scan consistency test
..

[integration tests] added scan consistency test

This is a small test to expose inconsistent reads when no timestamp
is propagated by the client to tablet servers.

This is in the context of KUDU-1679 Propagate timestamps for scans.

Change-Id: I88e0d4e244be8abcef18e0b8317bacfa9bf272cb
---
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/consistency-itest.cc
4 files changed, 386 insertions(+), 0 deletions(-)


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

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

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

2016-11-17 Thread Dinesh Bhat (Code Review)

Dinesh Bhat has posted comments on this change.

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
..


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5096/3/src/kudu/common/timestamp.h
File src/kudu/common/timestamp.h:

Line 109:   return rhs < lhs;
> Nope, this is how you define '>' operator using '<': you just swap argument
oh yeah, I missed that :), thanks.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..


Patch Set 3:

Chatted offline with MJ from the Impala team. He asked me to make it a WARNING 
instead of returning a bad Status in the case that Impala inits SASL but 
doesn't initialize the SASL mutexes. Both Impala and Kudu through 1.0 neglected 
to set up mutexes and have never seen a problem, so we don't think it's 100% 
critical. And there's some fear it could cause an unexpected perf issue to 
enable it now. So, we want to stage the change -- this patch will just make it 
a WARNING and unbreak the integration, and then they'll separately test the 
sasl_set_mutex() addition separately.

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

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

[kudu-CR] KUDU-1749. Allow callers to disable SASL initialization

2016-11-17 Thread Todd Lipcon (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1749. Allow callers to disable SASL initialization
..

KUDU-1749. Allow callers to disable SASL initialization

The Kudu client makes use of SASL, as do other applications such as
Impala that use the client. This means that both Kudu and those
applications are expected to initialize SASL.

The sasl_*_init() functions themselves are safe to call multiple times
(they have no effect). However, sasl_set_mutex() is not safe to call
multiple times, nor is it safe to call after another library has already
begun using the SASL libraries.

IMPALA-4479 describes a crash in SaslMutexUnlock() which is likely
caused by this issue: Impala has initialized the SASL library with no
call to sasl_set_mutex(), and then during Kudu initialization we set the
mutex functions to our own. Then when we call sasl_dispose(), SASL tries
to acquire an internal mutex which was previously allocated to a garbage
value 0x1 by the built-in "fake" mutex implementation. This crashes when
we attempt to access it using our real mutex implementation.

The fix has a few parts:
- we have a new API which allows embedders to disable initialization of
  SASL.
- we try to auto-detect the case in which the client has fully
  initialized SASL including providing a mutex implementation. In that
  case, we skip initialization but log a warning that the caller should
  disable it explicitly.
- if we detect that it's initialized but no mutex implementation is
  provided, we'll log a warning on first usage of SASL.
- if they disable initialization but didn't initialize SASL on their
  own, we'll return an error on first usage.

The one sketchy thing about this patch is that the SASL API doesn't give
a 100% public way to determine its initialization state. However, it
does export a symbol which we can import that provides the necessary
information. This symbol is marked as an API, so we expect it to be
stable enough for usage, at least across the range of supported
operating systems.

Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
---
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/sasl_rpc-test.cc
5 files changed, 183 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54d5ecfb2526852003f1ea249d1d2a8e6c0e91cd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] el6: fix krb5 realm workaround for static builds

2016-11-17 Thread Todd Lipcon (Code Review)

Todd Lipcon has posted comments on this change.

Change subject: el6: fix krb5 realm workaround for static builds
..


Patch Set 4:

(4 comments)

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

PS4, Line 32: important executables (tserver, master, and 'kudu' tool) 
> What about one-off executables like tpch, tpch_real_world, etc? And the cli
yea, I'll rephrase this to say that it's the executables that are used by tests


http://gerrit.cloudera.org:8080/#/c/5100/4/CMakeLists.txt
File CMakeLists.txt:

PS4, Line 945: Binaries which are used by tests need to link this in.
> Can you update this to be very precise about what kind of binaries need it?
Done


Line 947: set(KRB5_REALM_OVERRIDE -Wl,--undefined=krb5_realm_override_loaded 
krb5_realm_override)
> This line causes the macOS build to fail, but it appears if you wrap it in 
Done


http://gerrit.cloudera.org:8080/#/c/5100/4/src/kudu/security/CMakeLists.txt
File src/kudu/security/CMakeLists.txt:

Line 22: target_link_libraries(krb5_realm_override glog dl)
> I don't think we want to link libdl on macOS.
Done


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

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

[kudu-CR] el6: fix krb5 realm workaround for static builds

2016-11-17 Thread Todd Lipcon (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: el6: fix krb5 realm workaround for static builds
..

el6: fix krb5 realm workaround for static builds

Here's another attempt at enabling the workaround for the numeric realm
problem on el6 (see ba2ae3de4a7c43ff2f5873e822410e066ea99667 for
background).

The previous attempt didn't succeed for static builds because we were
setting LD_PRELOAD to the non-absolute path of the override library.
This worked OK in debug builds because our binary RUNPATHs included the
build/lib/ directory. Static builds didn't have such a RUNPATH and thus
the LD_PRELOAD was unable to locate the specified shared object.

The two options to fix this were (1) determine and specify an absolute
path, or (2) build this workaround into all of our binaries rather than
try to preload it at runtime.

The issue with #1 is that, when we get to Java-based testing, it would
have been inconvenient to determine the absolute path of the override
library. In addition, it would make test binaries non-relocatable, which
is somewhat of a hassle.

So, this takes approach #2. The build incantations are a bit nasty. In
order to make sure that the symbol shows up everywhere, and shows up
before the krb5 library, and doesn't show up more than once (to avoid
ODR violations), we have to manually add it to the link list for the
the executables that need the override. This includes the tests (and
thus the 'test_main' library) as well as any executables which are run
by the tests ((tserver, master, and 'kudu' tool).

In addition, we need to make sure that the linker doesn't entirely skip
the override binary. I accomplished this using the '--undefined' linker
flag.

I tested this patch using both sasl_rpc-test and
external_mini_cluster-test on an el6 box in both debug (dynamic linked)
and release (static-linked) builds. I also fully built both build types
to ensure there were no linker issues in unrelated tests.

Change-Id: I050d03d58658b650891566b9f955c867b15731e0
---
M CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/security/CMakeLists.txt
R src/kudu/security/krb5_realm_override.cc
M src/kudu/tools/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
M src/kudu/util/test_main.cc
M src/kudu/util/test_util.cc
11 files changed, 39 insertions(+), 38 deletions(-)


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

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

[kudu-CR] KUDU-1722 - Range Partition Failure on UNIXTIME MICROS

2016-11-17 Thread Jordan Birdsell (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1722 - Range Partition Failure on UNIXTIME_MICROS
..

KUDU-1722 - Range Partition Failure on UNIXTIME_MICROS

Errors are returned when attempting to add range partition bounds
for a unixtime_micros value when specifying bound types that are
not the default. This is happening because the logic to convert the
bounds to defaults (inclusive for Lower and exclusive for Upper)
converts the UNIXTIME_MICROS value to an INT64. This patch addresses
this issue and includes a test.

Change-Id: I47d23d184a6b73c5c0cdd04519821c4e7eb53ecb
---
M src/kudu/client/client-test.cc
M src/kudu/common/partition.cc
2 files changed, 39 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I47d23d184a6b73c5c0cdd04519821c4e7eb53ecb
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

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

2016-11-17 Thread Maxim Smyatkin (Code Review)

Maxim Smyatkin has uploaded a new change for review.

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

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 pb_util.h.
In a couple of places it led to string concatenations, but I suppose it's
not that bad for overal 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
(see https://gerrit.cloudera.org/#/c/5007/).

Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
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_session-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
11 files changed, 31 insertions(+), 29 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I52a794cc16d008570039d276ab32e6a26829bc53
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin

64 matches

Mail list logo