[kudu-CR] Doxygen for C++ client API

[Alexey Serbin (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: Doxygen for C++ client API
..

Doxygen for C++ client API

If doxygen is available, build 'doxygen' taget to generate
Doxygen docs from client.h and other Kudu C++ client API files,
After the target is built, open
${CMAKE_CURRENT_BINARY_DIR}/docs/doxygen/client_api/html/index.html
in your favorite browser to see the generated documentation.

If doxygen is available, the 'docs' target generates
doxygen documentaion as well since it depends on the 'doxygen' target.

Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
---
M CMakeLists.txt
A docs/support/doxygen/client_api.doxy.in
M src/kudu/client/client.h
M src/kudu/util/CMakeLists.txt
M thirdparty/download-thirdparty.sh
5 files changed, 1,298 insertions(+), 721 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 

[kudu-CR] Doxygen for C++ client API

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Doxygen for C++ client API
..


Patch Set 12:

(42 comments)

http://gerrit.cloudera.org:8080/#/c/3619/12/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 92: /// Only the first invocation has effect; subsequent invocations are
> This needs to be 'any effect' or preferably 'an effect'
Done


Line 103: /// Only the first invocation has any effect; subsequent invocations 
are
> whatever you decide about 'an effect' above should probably be mirrored her
Done


Line 145: /// @brief A "factory" for the KuduClient objects.
> s/for the/for
Done


Line 147: /// This class is used to create instances of KuduClient class
> s/of/of the
Done


Line 162:   ///   RPC addresses of masterts to add.
> s/masterts/masters
Done


Line 175:   /// Set default timeout for administrative operations.
> s/default/the default
Done


Line 195:   /// Create client object.
> s/client/a client
Done


Line 200:   ///   The newly created instance wrapped into a shared pointer.
> s/into/in
Done


Line 233: /// In order to actually access data on the cluster, callers must 
first
> s/actually access data on/write data to
Done


Line 239: /// @todo Cluster administration functions are likely to be in this 
class
> I'm not sure we actually want to change our TODO syntax; it's pretty consis
@todo is also easy to grep, right?  Using @todo gives benefits of easy tracking 
those even in the generated documentation; e.g., see 
https://alexeyserbin.github.io/todo.html

I prefer to keep @todo for the external-facing APIs like this to allow better 
visibility.


Line 247:   /// @return Pointer to newly created instance; it's the caller's
> For consistency you should probably drop the contraction here and below.
Done


Line 251:   /// Check whether CreateTable operation is in-progress.
> s/CreateTable/ a create table
Done


Line 269:   /// Create a KuduTableAlterer object.
> It's probably best to be consistent about object vs. instance, for example 
Done


Line 272:   ///   Name of the table to watch for.
> s/watch for/alter
Done


Line 280:   ///   Name of the table in question.
> remove 'in question' here and below, I don't think it adds any clarity.
Done


Line 310:   ///   Substring filter to use; empty sub-string filter matches any 
tables
> s/any tables/all tables.
Done


Line 326:   /// If the table has not been opened before, then open the table
> I think this paragraph can be entirely removed, the first sentence is misle
Done


Line 359:   /// @return @c true iff client was configured to talk to multiple
> s/was/is
Done


Line 373:   /// Get highest HybridTime timestamp observed by the client.
> s/highest/the highest
Done


Line 388:   /// To use this the user must obtain the HybridTime encoded 
timestamp from
> The HybridTime encoded timestamp should be obtained from another client's K
Done


Line 442:   ///   Name of the target table -- it is copied.
> I would get rid of the note about the copy, it really shouldn't matter to c
Done


Line 443:   /// @return Reference to the modified object.
> s/object/table creator
Done


Line 445:   /// @remark Calling this method and setting name for the table-to-be
> s/name/the name
Done


Line 469:   /// For each set of hash partitions added to the table, the total 
number of
> s/table partitions/tablets
Done


Line 472:   /// with 4 and 5 buckets respectively, the total number of table 
partitions
> s/table partitions/tablets
Done


Line 505:   /// call this method with an empty vector and set no split rows an 
no hash
> s/an/and
Done


Line 517:   /// If no range split rows are added, no range pre-splitting is 
performed.
> Remove this sentence; it's not true anymore since it can be done via range 
Done


Line 559:   /// Set the number of replicas for each tablet in the table.
> Referring to tablets in this is probably going to cause confusion.  I think
Done


Line 567:   /// Set the timeout for the operation.
> s/operation/table creation operation/
Done


Line 622: /// This class is also a factory for write operation for on the table.
> s/for on/on
Done


Line 675:   /// This method creates new instance of comparison predicate which
> s/of/of a
Done


Line 700:   /// @return The KuduClient object associated with the table.
> add a note that the caller should not free the client.
Done


Line 725: /// Create a new instance of alterer using 
KuduClient::NewTableAlterer().
> s/of alterer/of a table alterer
Done


Line 756:   /// Alter an existing column.
> Add a note that the column may not be in the primary key.
Done


Line 764:   /// Drops an existing column from the table.
> Add a note that they column may not be in the primary key.
Done


Line 771:   /// Set timeout for the alteration operation.
> s/timeout/a timeout
Done


Line 814: /// @brief This class represents an error which occurred in a given 
operation.
> s/operation/write operation
Done


Line 

[kudu-CR] async background flush provision for C++ client

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: async background flush provision for C++ client
..


Patch Set 2:

(59 comments)

Thank you for review.

I addressed most of the comments, and I'm working on testing the change more 
thoroughly by all means (more tests will follow) and also looking at the other 
option of implementing background flushing.  Adar recommended to look at the 
option to utilize one of the reactor threads for that purpose.

I'll post an update on this change, but more changes are expected to appear 
soon.

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

PS2, Line 7: provision
> nit: remove provision
Done


Line 7: async background flush provision for C++ client
> nit: capitalize (Async)
Done


PS2, Line 10: is
> nit: avoid using the passive voice. i.e. "KuduSession starts a background f
Done


PS2, Line 13: virtual
> what's a virtual buffer?
There isn't a single buffer as is, there is a set of buffers used by multiple 
batchers.  Will add clarification comment.


Line 16: The flush criteria is based on buffer size for not-yet-scheduled
> I'm having trouble parsing this sentence.
Done


PS2, Line 17: operatations
> nit: type
Done


PS2, Line 20: operatations
> nit: typo
Done


PS2, Line 23: KUDU
> Mention this issue in the commit message title.
Done


http://gerrit.cloudera.org:8080/#/c/3668/2/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:

Line 267: #ifndef NDEBUG
> spurious change
It's not spurious, it fixes compilation warning of unused variables for debug 
builds.


Line 383: 
> spurious change
Done


Line 553:   //since the other time it's called in KuduSession::Apply()
> nit: rephrase. How about:
Done


http://gerrit.cloudera.org:8080/#/c/3668/2/src/kudu/client/batcher.h
File src/kudu/client/batcher.h:

Line 109:   // Return number of bytes used for batcher buffer
> nit: "Returns the"
I see most of comments in this file does not use third person singular form.  
The only exception is external_consistency_mode().  Are you sure it should be 
'Returns the number', but not 'Return the number'?


PS2, Line 216: //
> no need for this comment
Done


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

Line 1991: // return an error with session running in any supported flush mode
> nit: period
Done


PS2, Line 1993: static
> not need for static.
Done


PS2, Line 1993: buffer_space_bytes_limit
> style: kBufferSpaceBytesLimit or BUFFER_SPACE_BYTES_LIMIT
Done


PS2, Line 1994: static
> same
Done


Line 1995:   static const KuduSession::FlushMode modes[] = {
> same
Done


Line 2001:   for (const auto mode: modes) {
> isn't 'auto' already const ?
Done


Line 2007: ASSERT_TRUE(s.IsIncomplete()) << "got unexpected status: " << 
s.ToString();
> nit: capitalize Got
Done


Line 2014:   // Applying a bunch of small rows without a flush should not 
result in
> this sentence is huge and hard to read. please break it down.
Done


Line 2016:   // since there is a flow control which makes Session::Apply() 
block and wait
> what: "a flow control"?
Done


Line 2018:   {
> no need for the extra { scope
Done


Line 2023:   ASSERT_OK(ApplyInsertToSession(session.get(), client_table_, 
1, 1, "x"));
> How this testing that Apply() is eventually blocking, or making sure that t
This test is to verify that there isn't an error about overrunning the buffer.  
Yes, it's not an explicit test to verify that Apply() is blocking.  Will 
address that separately.


Line 2029: // operations are put into the queue flushed after some time.
> you mean : "into the queue and flushed" right?
Done


Line 2030: // The exact timeout interval is implementation-dependent,
> which timeout interval?
Done


Line 2031: // but 100 ms is a good upper limit anyways.
> s/anyways/anyway
Done


Line 2033:   {
> no need for the extra {
Done


Line 2041: SleepFor(MonoDelta::FromMilliseconds(100));
> loop instead of wait, otherwise this will be brittle in jenkins
Done


http://gerrit.cloudera.org:8080/#/c/3668/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:

Line 705:   "Cannot change flush mode when writes are buffered");
> nit period
Done


Line 707:   data_->SetFlushMode(m);
> would it simplify things to now allow the flush mode to change if there is 
I think we can allow the flush mode to change regardless presence of 
buffered/pending write operations, but it requires proper design and some 
additional logic to be implemented.  I would try to address this in a separate 
patch/change-list.


Line 728:   return data_->SetBufferBytesLimit(size);
> why not use the same method name?
Because from internal perspective there is no context of any mutation.  It's 
just a buffer that accommodates byte representation of write operations.

>From the other hand, the outer (API-facing) name cannot be changes now without 
>braking 

[kudu-CR] KUDU-1492: Show column encodings/compression on table page in master

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1492: Show column encodings/compression on table page in 
master
..


Patch Set 2:

(6 comments)

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

Line 9: This displays column  atributes for the table schema in picture.
typo: atributes --> attributes


PS2, Line 9:  
nit: an extra space


Line 13: release. For eg, current release has PLAIN_ENCODING for encoding
nit: For eg --> E.g. ?


http://gerrit.cloudera.org:8080/#/c/3667/2/src/kudu/server/webui_util.cc
File src/kudu/server/webui_util.cc:

PS2, Line 40:   
nit: Is this still a tab?  If yes, consider expanding it with spaces to comply 
with code style.


Line 54:   string encoding = EncodingType_Name(attrs.encoding);
nit: consider replacing with const reference:

const string& encoding = ...


Line 55:   string compression = CompressionType_Name(attrs.compression);
Ditto -- consider replacing with const reference.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I990a8d790ab71a05be04f0b7468b5da0894478e8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] client/sample.cc: fixed a couple of crashes

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: client/sample.cc: fixed a couple of crashes
..


Patch Set 1:

(4 comments)

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

Line 11: while logging some messages from terminating reactor threads.
> is there another way we can fix this without forcing callers to uninstall t
I could think of making the logger callback static with lifespan longer than 
those objects in the main() scope.  However, I'm not sure that will guarantee 
the desired behavior: I'm not sure there is a guarantee the reactor threads 
will be dead at time destruction of that static object.

Another thing I'm thinking is to create a wrapper object which would install 
the logger callback in constructor and un-install the callback in its 
destructor (no exceptions in case of failures, though -- only messages to 
std::cerr).

But those are about working around the real issue of shutting down reactor 
threads independently in the background.  Probably, the destructor of 
KuduClient should await for completion of those reactor threads instread.


Line 11: while logging some messages from terminating reactor threads.
> As with the below, any ideas as to why the precommit test didn't experience
There is a catch: I modified the sample.cc to run with different parameters.  
That exposed this bug.  I built the source in debug configuration.  If needed, 
I can send you the patch.

Basically, I played around the sample.cc building different scenarios to run 
against the original client library code and code with my recent modifications. 
 The issues fixed in the patch appear even in client library with no 
AUTO_BACKGROUND_FLUSH support.


Line 13: Fixed issue with an attempt to access non-existing element
> hmm, the bug fix looks reasonable, but I'm curious why we don't see this cr
Please see the response for the prior comment: in essence, I run modified 
sample.cc and that exposed the bug.


Line 13: Fixed issue with an attempt to access non-existing element
> I'm also surprised. front() on an empty vector is undefined behavior, so ma
I found that going through different scenario in sample.cc, basically modified 
the sample.cc code.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5fa3b812e6402a113bf5e432a3a451dc4cc3735
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Doxygen for C++ client API

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Doxygen for C++ client API
..

Doxygen for C++ client API

If doxygen is available, build 'doxygen' taget to generate
Doxygen docs from client.h and other Kudu C++ client API files,
After the target is built, open
${CMAKE_CURRENT_BINARY_DIR}/docs/doxygen/client_api/html/index.html
in your favorite browser to see the generated documentation.

If doxygen is available, the 'docs' target generates
doxygen documentaion as well since it depends on the 'doxygen' target.

Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
---
M CMakeLists.txt
A docs/support/doxygen/client_api.doxy.in
M src/kudu/client/client.h
M src/kudu/util/CMakeLists.txt
M thirdparty/download-thirdparty.sh
5 files changed, 1,294 insertions(+), 721 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 

[kudu-CR] Doxygen for C++ client API

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Doxygen for C++ client API
..


Patch Set 9:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/3619/9/src/kudu/client/client.h
File src/kudu/client/client.h:

> 1. OK, then I think reverting back to one space makes sense.
NP: the crucial part is that we've got some progress :)


Line 432: class KUDU_EXPORT KuduTableCreator {
> I'm referring to how, in some of the old comments here, there were one-word
Oh, I see.  Yes, I also think it's nice to have that information.  I'm about to 
add that as preconditions for appropriate objects.  Please stay tuned -- will 
post with next revision of the patch.


http://gerrit.cloudera.org:8080/#/c/3619/11/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 767:   ///
> Nit: operation
Done


Line 1079:   /// flushes itself inline.
> Will this also include the "Flush any pending writes" sentence from Flush()
Good catch -- yes, that should be fixed.  Done.


Line 1332:   /// @note This method is unstable, and for internal use only.
> Nit: begin scanning
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 
Gerrit-HasComments: Yes

[kudu-CR] C++ client: fix on KuduSession::GetPendingErrors()

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: C++ client: fix on KuduSession::GetPendingErrors()
..

C++ client: fix on KuduSession::GetPendingErrors()

If re-using the same std::vector instance as a placeholder
for repeated calls to KuduSession::GetPendingErrors(), irrelevant
error information can be pushed into the session error container.
Clean the placeholder container before swapping it with current
error container of the KuduSession.

Also, make it possible to pass NULL for the 'overflowed' out parameter.

Change-Id: Iafee46a6135ac2096d1f8b690da5b084a15eb73e
---
M src/kudu/client/error_collector.cc
1 file changed, 7 insertions(+), 2 deletions(-)


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

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

[kudu-CR] Add time/watermark based garbage collection to ResultTracker

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add time/watermark based garbage collection to ResultTracker
..


Patch Set 13:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/3628/13/src/kudu/rpc/exactly_once_rpc-test.cc
File src/kudu/rpc/exactly_once_rpc-test.cc:

Line 64: } // namespace
nit: anonymous namespace, otherwise it looks like namespace name is missing.


Line 94: RetriableRpcStatus status;
nit: is it possible to make status local variable for related scopes only?  It 
does not look like the status variable needs to go though all the scopes to 
accumulate some changes on its initial value.


Line 243:   SleepFor(MonoDelta::FromMilliseconds(rand() % 10));
Do we want these pseudo-random numbers to repeat in the same sequence for every 
run of the test?  If not, consider initializing random with seed, i.e. calling 
srandom(time(nullptr)) or something like that before the while() cycle.


Line 257:   rand() % (2 * FLAGS_remember_responses_ttl_msecs)));
Ditto for the rand().


Line 259:   SleepFor(MonoDelta::FromMilliseconds(rand() % 10));
Ditto for the rand().


Line 270:   // Stubbornly sends the same request to the server, this should 
observe three states
Nit: period is missing in the end of the sentence.


Line 271:   // The request should be successful at first, then its result 
should be gc'd and the the
typo: strayed extra 'the'.


Line 297: SleepFor(MonoDelta::FromMilliseconds(rand() % 10));
Ditto for the rand().


Line 513: // This test creates a thread continuously makes requests to the 
server, some lasting longer
nit: makes --> making


http://gerrit.cloudera.org:8080/#/c/3628/13/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:

Line 262: // The timestamp of the last time this CompletionRecord was 
updated.
nit: 'The timestamp of the last CompletionRecord update.'  ?


Line 301: SequenceNumber stale_before_seq_no;
Is it OK to leave it uninitialized in the constructor (I didn't see it was 
initialized).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Doxygen for C++ client API

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Doxygen for C++ client API
..

Doxygen for C++ client API

If doxygen is available, build 'doxygen' taget to generate
Doxygen docs from client.h and other Kudu C++ client API files,
After the target is built, open
${CMAKE_CURRENT_BINARY_DIR}/docs/doxygen/client_api/html/index.html
in your favorite browser to see the generated documentation.

If doxygen is available, the 'docs' target generates
doxygen documentaion as well since it depends on the 'doxygen' target.

Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
---
M CMakeLists.txt
A docs/support/doxygen/client_api.doxy.in
M src/kudu/client/client.h
M src/kudu/util/CMakeLists.txt
M thirdparty/download-thirdparty.sh
5 files changed, 1,271 insertions(+), 721 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 

[kudu-CR] doxygen for C++ client API

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: doxygen for C++ client API
..


Patch Set 9:

(64 comments)

http://gerrit.cloudera.org:8080/#/c/3619/9/src/kudu/client/client.h
File src/kudu/client/client.h:

PS9, Line 91: Before a callback is registered, all internal client log events
: /// are logged to the stderr.
> Nit: why combine this sentence into the previous paragraph instead of leavi
Done


Line 131: ///   Signal number to use for internal
> Nit: for internal what? Maybe you meant "Signal number to use internally"?
Done


PS9, Line 222: Each KuduClient instance is sandboxed with no
 : /// global cross-client state.
> Not related to your change, but this isn't strictly true anymore. Basically
Done


Line 247:   /// @return Pointer to newly created instance: it's the caller's
> Nit: semi-colon is more appropriate here.
Done


Line 277:   /// Check if table alteration is in process
> Nit: in-progress, to be consistent with IsCreateTableInProgress.
Done


PS9, Line 282:   ///   Set to @c true if an AlterTable operation is in-progress;
 :   ///   set to @c false otherwise
> Nit: can you reword to be consistent with IsCreateTableInProgress, since th
Done


Line 287:   /// Get scheme for a table
> Nit: schema, not scheme.
Done


Line 290:   ///   Name of the table if question
> Nit: in question
Done


Line 292:   ///   Raw pointer to the schema object: caller gets ownership
> Nit: semi-colon.
Done


Line 310:   ///   Substring-filter to use; empty sub-string filter matches any 
tables
> Nit: Substring filter
Done


Line 320:   ///   Set only on success: set to @c true iff table exists
> Nit: semi-colon
Done


PS9, Line 326:   /// If the table has not been opened before, then open the 
table
 :   /// with the given name.  If the table has not been opened 
before
 :   /// for this client, this will do an RPC to ensure that the 
table exists
 :   /// and look up its schema.
> Also not related to this change, but the RPC and schema lookup are always d
Done


Line 346:   /// @return A new session object: caller is responsible for 
destroying it.
> Nit: semi-colon.
Done


Line 359:   /// @return @c true iff it's a multi-master session
> This isn't related to sessions per-se, it's true if the client was configur
Done


Line 365:   /// @return Default timeout for RPC operations.
> Nit: we do distinguish between "operations" and "RPCs". The former are logi
Done


Line 376:   /// by this client.  Check KuduScanner for more details on 
timestamps.
> Nit: why the change from "See KuduScanner" to "Check KuduScanner"? Was the 
I was playing with @see directive here, but doxygen itself inserts hyperlink to 
KuduScanner here.  I dropped the '@' after realizing that.  OK, now it's back 
to 'See'.


PS9, Line 385: To use this the user must obtain
 :   /// the HybridTime encoded timestamp from the first client with
 :   /// KuduClient::GetLatestObservedTimestamp() and then set it
 :   /// in the new client with this method.
> Nit: let's separate this into a new paragraph.
Done


Line 432: class KUDU_EXPORT KuduTableCreator {
> The "Required" and "Optional" tags are gone. I think they were useful; can 
I don't understand what "Required" and "Optional" tags are in this context.  
Could you clarify?


Line 438:   /// This method must be called at an object before .
> This sentence is unclear, and there's an extra space at the end.
Done


Line 447:   /// Set the schema with which to create next table.
> Nit: the subsequent builder methods refer to the table to be created as sim
Done


PS9, Line 462: two
> Nit: to be consistent with the rest of this paragraph, use '2' instead of '
Done


PS9, Line 476:   /// This method takes a seed value, which can be used to 
randomize the
 :   /// mapping of rows to hash buckets.  Setting the seed may 
provide some
 :   /// amount of protection against denial of service attacks 
when the hashed
 :   /// columns contain user provided values.
> Can you add a small note at the beginning explaining that this is exactly t
Done


PS9, Line 482: Names of columns to build partition
> To be consistent with set_range_partition_columns(), how about "to use for 
Done


Line 499:   ///   Name of columns to use for partitioning.  Every column must be
> Nit: Names of columns
Done


PS9, Line 524:   /// Add a partition range based on lower and upper bounds.
 :   ///
 :   /// Add a partition range bound to the table with an inclusive 
lower bound
 :   /// and exclusive upper bound.
> These two sentences are almost the same, maybe we can just the second sente
Done


Line 533:   /// If this method is not called, the table's range is be unbounded.
> Nit: "is be"?
Done


Line 539:   ///   on the table range.  If a column of the @c lower_bound row 

[kudu-CR] doxygen for C++ client API

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: doxygen for C++ client API
..

doxygen for C++ client API

If doxygen is available, build 'doxygen' taget to generate
Doxygen docs from client.h and other Kudu C++ client API files,
After the target is built, open
${CMAKE_CURRENT_BINARY_DIR}/docs/doxygen/client_api/html/index.html
in your favorite browser to see the generated documentation.

If doxygen is available, the 'docs' target generates
doxygen documentaion as well since it depends on the 'doxygen' target.

Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
---
M CMakeLists.txt
A docs/support/doxygen/client_api.doxy.in
M src/kudu/client/client.h
M src/kudu/util/CMakeLists.txt
M thirdparty/download-thirdparty.sh
5 files changed, 1,283 insertions(+), 717 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 

[kudu-CR] doxygen for C++ client API

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: doxygen for C++ client API
..


Patch Set 9:

(1 comment)

> One thing we should add to this (it can be a follow-up commit).
 > 
 > It needs to be integrated into make_site.sh and the web site
 > itself.

Yes, certainly -- I will add that integration is a follow-up commit.

http://gerrit.cloudera.org:8080/#/c/3619/9/src/kudu/client/client.h
File src/kudu/client/client.h:

> High level formatting questions:
1.  It's a purely stylistic change, it's not related to doxygen requirements at 
all.  So, if there is some sort of implicit rule of using just one space before 
starting a new sentence, I'll revert it back.

2.  The idea was to have a brief (i.e. one sentence) a method/class summary, so 
no period would be needed since after @brief message there should be an empty 
line.  However, for parameters and return values there is no guarantee the 
description is brief, because it's the only description around for those.  So, 
@brief would not be ended with a period, but @return and @param would.  If that 
seems to be an inconsistency, let's end @brief message with a period as well.

3. It's because some methods were treated correspondingly after I got feedback 
from Todd in the kudu-dev mailing list :)  I.e., I removed @brief comments 
which were almost one-to-one copy of the @return comments.  I'll take care of 
the rest of those simple getters leaving only @return comment.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 
Gerrit-HasComments: Yes

[kudu-CR] async background flush provision for C++ client

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: async background flush provision for C++ client
..


Patch Set 1:

> What was the rationale behind using a dedicated thread to manage
 > background flushing? I think synchronization would be net simpler
 > if instead, we reused the messenger's reactor threads for
 > background flushing. They're already used to run callbacks whenever
 > a Proxy::FooAsync() call is made, and you can use 
 > Messenger::ScheduleOnReactor()
 > to schedule the background flushing to happen at some time in the
 > future.
 > 
 > Some other arguments in favor of using reactor threads:
 > 1. It reduces the thread count  of the client.
 > 2. Structurally it makes the implementation similar to the Java
 > client.

I thought messenger reactor threads are for messaging, and I was not so 
familiar with that part of code.  OK, I'll explore that option in this context.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idfde05bef342db24990c6e3da3b0270c3bb37a9d
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1492: Show column encodings/compression on table page in master Test results are in JIRA: https://issues.apache.org/jira/browse/KUDU-1492

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1492: Show column encodings/compression on table page in 
master Test results are in JIRA: https://issues.apache.org/jira/browse/KUDU-1492
..


Patch Set 1: Code-Review+1

(2 comments)

LGTM, just a couple questions.

http://gerrit.cloudera.org:8080/#/c/3667/1/src/kudu/server/webui_util.cc
File src/kudu/server/webui_util.cc:

Line 35:std::stringstream* output) {
This is not part of this change, but just an additional comment.  Does it make 
sense to change type of output into more narrower type ostringstream since the 
method is supposed only to output some data in there?


Line 56: *output << 
Substitute("$0$1$2$3"
I, as a reader, would appreciate some text example of the output in the 
comment.  Would it make sense?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I990a8d790ab71a05be04f0b7468b5da0894478e8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] async background flush provision for C++ client

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: async background flush provision for C++ client
..

async background flush provision for C++ client

Implemented AUTO_FLUSH_BACKGROUND for C++ client library.
A dedicated background flusher thread is started by KuduSession
instance for this purpose.  In AUTO_FLUSH_BACKGROUND mode the
KuduSession::Apply() method blocks if there is not enough place
in the virtual buffer.  The limit on the buffer size is set by
KuduSession::SetMutationBufferSpace().

The flush criteria is based on buffer size for not-yet-scheduled
session's write operatations and timeout.

The flow control criteria in KuduSession::Apply() is based on
total buffer size of session's write operatations.

Addressed the following JIRA issues:
  KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
  KUDU-1376 KuduSession::SetMutationBufferSpace is not defined

Change-Id: Idfde05bef342db24990c6e3da3b0270c3bb37a9d
---
M src/kudu/client/batcher.cc
M src/kudu/client/batcher.h
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/error_collector.cc
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
7 files changed, 432 insertions(+), 103 deletions(-)


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

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

[kudu-CR] doxygen for C++ client API

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: doxygen for C++ client API
..

doxygen for C++ client API

If doxygen is available, build 'doxygen' taget to generate
Doxygen docs from client.h and other Kudu C++ client API files,
After the target is built, open
${CMAKE_CURRENT_BINARY_DIR}/docs/doxygen/client_api/html/index.html
in your favorite browser to see the generated documentation.

If doxygen is available, the 'docs' target generates
doxygen documentaion as well since it depends on the 'doxygen' target.

Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
---
M CMakeLists.txt
A docs/support/doxygen/client_api.doxy.in
M src/kudu/client/client.h
M src/kudu/util/CMakeLists.txt
M thirdparty/download-thirdparty.sh
5 files changed, 1,294 insertions(+), 718 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 9
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: Mike Percy 

[kudu-CR] doxygen for C++ client API

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: doxygen for C++ client API
..


Patch Set 8:

(2 comments)

Will send an update shortly.

http://gerrit.cloudera.org:8080/#/c/3619/8/CMakeLists.txt
File CMakeLists.txt:

Line 989: add_custom_target(doxy_install_client_alt_destdir
> Interesting. I think the kudu_client_exported problem is due a misuse of ad
Perfect -- it works now.  So, it was a bug, not a feature.  Thank you for 
fixing this!


Line 992:   WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/src/kudu/client
> Running make out of anywhere but the top-level directory isn't something mo
OK, that sounds reasonable.


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

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

[kudu-CR] doxygen for C++ client API

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: doxygen for C++ client API
..


Patch Set 8:

(4 comments)

Please see the responses in-line.  Once the questions are cleared, I'll post 
the next version.

Thanks!

http://gerrit.cloudera.org:8080/#/c/3619/8/CMakeLists.txt
File CMakeLists.txt:

Line 989: add_custom_target(doxy_install_client_alt_destdir
> Maybe there's no real 'all' target after all. What about depending on 'kudu
Yesterday I tried dependency on 'kudu_client_exported' as well :)  The problem 
is that 'kudu_client_exported' does not have proper dependencies and if 
depending on this target, sometimes the following error appeared:

/Users/aserbin/Projects/kudu/src/kudu/util/version_info.cc:22:10: fatal error: 
  'kudu/generated/version_defines.h' file not found

Instead of fixing that 'kudu_client_exported' problem (I thought it might be a 
feature) I decided to go with calling the 'all' target explicitly since I'm 
calling the 'install' target anyways.

Please let me know if you think it's still worth continuing exploring this path 
and fixing those issues down the road.


Line 990:   COMMAND ${CMAKE_COMMAND} -E remove_directory 
${DOXY_CLIENT_DESTDIR}
> Hmm, OK. I was hoping that a second "make install" would automatically clea
'make install' does not clean old contents in the target DESTDIR.  At least as 
I checked it, if some extra file from previous 'make install' is left, the new 
'make install' leaves the extra file in place, overwriting only files which are 
common in old and new runs.


Line 992:   WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/src/kudu/client
> You don't have to worry about that, though; "make install" only installs th
Yes, but why to depend on that at all and spend extra-time fixing that if 
something changes in future in the top-level 'install' target?  Why not to use 
the client-specific target if it already exists and does exactly what is 
needed?  I think possibility of affecting changes in the top-level 'install' 
target's behavior is higher compared with possibility of changes in the 
client-specific target. 

I'm hesitant to change that due to the reasons above, but I might miss 
something.  Please let me know if you still think it's worth changing to run 
'make install' at the top-level.


http://gerrit.cloudera.org:8080/#/c/3619/8/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

PS8, Line 63: if [[ "$FILENAME" =~ \.zip$ ]]; then
:   if ! unzip -q "$FILENAME"; then
: echo "Error unzipping $FILENAME, removing file"
: rm "$FILENAME"
: continue
:   fi
: elif [[ "$FILENAME" =~ \.(tar\.gz|tgz)$ ]]; then
> OK, I see. Let's keep them, then.
All right, I see.  But if there is additional vote from another reviewer to 
keep the original, I'll revert these changes.


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

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

[kudu-CR] doxygen for C++ client API

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: doxygen for C++ client API
..


Patch Set 8:

(6 comments)

Thank you for review!

http://gerrit.cloudera.org:8080/#/c/3619/8/CMakeLists.txt
File CMakeLists.txt:

Line 983: set(DOXY_SUBDIR ${CMAKE_CURRENT_BINARY_DIR}/docs/doxygen)
> Could you first log what the location of doxygen is? Maybe logging DOXYGEN_
The find_package(Doxygen) outputs information on location of doxygen binary 
found along with its version.  Would be it enough?  Or it's better to output 
doxygen binary location every time the 'doxygen' target is built?


Line 989: add_custom_target(doxy_install_client_alt_destdir
> What if doxy_install_client_alt_destdir depended on 'all'? Then could you d
The 'all' and 'install' steps are here is for kudu-client sub-dir, actually.  
But yes, it might depend on 'all' target for the top-level (however, it could 
fire back if 'all' had 'docs' as the dependency :)).  Actually, I was down that 
path already :)  However, when I tried to introduce such a dependency, cmake 
failed stating that 'all' target did not exist.

If you know how to deal with that issue, please let me know -- I will gladly 
update this part.


Line 990:   COMMAND ${CMAKE_COMMAND} -E remove_directory 
${DOXY_CLIENT_DESTDIR}
> Why do we need to remove the old directory first?
This is for cleaning some stale stuff if it's left around when make was 
interrupted in one of prior runs.  Since this is a phony target, no 
auto-removal is possible on interruption.  Does it make sense?


Line 992:   WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/src/kudu/client
> Why does the working directory need to be modified for this command?
Because we need only client-related parts to be installed into the alternative 
destdir, nothing else: we are not interested in populating the directory 
structure with something which is not relevant to the client API.


Line 997:   COMMAND ${CMAKE_COMMAND} -E remove_directory 
${DOXY_CLIENT_DESTDIR}
> Why this?
This is a clean-up of the temporary stuff that was created.  The original idea 
was to use mktemp to create a temporary dir and then get rid of it, but it went 
into too complex cmake function and I abandoned it.  So, it's just some remnant.

We can leave with having that around, that's OK.  Actually, after some 
consideration I see it could be useful, especially if the installed files were 
different from those we have in the source tree -- it would allow to refer to 
the actual data if resolving warning/errors coming from doxygen.

I'll change this.


http://gerrit.cloudera.org:8080/#/c/3619/8/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

PS8, Line 63: if [[ "$FILENAME" =~ \.zip$ ]]; then
:   if ! unzip -q "$FILENAME"; then
: echo "Error unzipping $FILENAME, removing file"
: rm "$FILENAME"
: continue
:   fi
: elif [[ "$FILENAME" =~ \.(tar\.gz|tgz)$ ]]; then
> Are these changes still useful? Or can they be reverted? They seem cosmetic
This change allows avoid running extra-processes to check for regex patterns.

Yes, they can be reverted.  I just though not running extra-processes was a 
good change.  Do you insist on reverting these?


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

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

[kudu-CR] doxygen for C++ client API

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: doxygen for C++ client API
..

doxygen for C++ client API

If doxygen is available, build 'doxygen' taget to generate
Doxygen docs from client.h and other Kudu C++ client API files,
After the target is built, open
${CMAKE_CURRENT_BINARY_DIR}/docs/doxygen/client_api/html/index.html
in your favorite browser to see the generated documentation.

If doxygen is available, the 'docs' target generates
doxygen documentaion as well since it depends on the 'doxygen' target.

Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
---
M CMakeLists.txt
A docs/support/doxygen/client_api.doxy.in
M src/kudu/client/client.h
M thirdparty/download-thirdparty.sh
4 files changed, 1,294 insertions(+), 716 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] client.h: doxygen comments for C++ API

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: client.h: doxygen comments for C++ API
..


Patch Set 5:

(1 comment)

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

Line 963:   find_package(Doxygen REQUIRED)
> Sure, if doxygen is in the thirdparty tree then that's fine. Out of curiosi
On my MacBookPro if running 'make -j8':

real0m16.457s
user1m8.855s
sys 0m6.126s

Plus a 8 seconds to generate GNU makefiles out of cmakefiles.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] client.h: doxygen comments for C++ API

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: client.h: doxygen comments for C++ API
..

client.h: doxygen comments for C++ API

To generate the Doxygen docs from client.h, run 'make doxydocs'.
Open $REPO_ROOT/docs/doxygen/html/index.html in your favorite browser
to see the generated documentation.  The 'docs' target generates
doxygen documentaion as well since it depends on the 'doxydocs' target.

Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
---
M CMakeLists.txt
A cmake_modules/FindDoxygen.cmake
M docs/.gitignore
A docs/support/doxygen/client.doxy
M src/kudu/client/client.h
5 files changed, 1,410 insertions(+), 717 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] client.h: doxygen comments for C++ API

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: client.h: doxygen comments for C++ API
..

client.h: doxygen comments for C++ API

To generate the Doxygen docs from client.h, run 'make doxydocs'.
Open $REPO_ROOT/docs/doxygen/html/index.html in your favorite browser
to see the generated documentation.  The 'docs' target generates
doxygen documentaion as well since it depends on the 'doxydocs' target.

Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
---
M CMakeLists.txt
M docs/.gitignore
A docs/support/doxygen/client.doxy
M src/kudu/client/client.h
4 files changed, 1,239 insertions(+), 717 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] client.h: doxygen comments for C++ API

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: client.h: doxygen comments for C++ API
..

client.h: doxygen comments for C++ API

To generate the Doxygen docs from client.h, run 'make doxydocs' target.
Open $REPO_ROOT/docs/doxygen/html/index.html in your favorite browser
to see the generated documentation.

Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
---
M CMakeLists.txt
M docs/.gitignore
A docs/support/doxygen/client.doxy
M src/kudu/client/client.h
4 files changed, 3,581 insertions(+), 716 deletions(-)


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

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

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 7: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3593/7/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 65: #include 
Is it really needed here?


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

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

[kudu-CR] Add a EraseKeyReturnSmartPtrValue to map-util

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add a EraseKeyReturnSmartPtrValue to map-util
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3595/3/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 711: return typename Collection::value_type::second_type();
It seems I missed it first time, but anyways: consider replacing 
Collection::value_type::second_type with Collection::mapped_type here.


Line 713:   typename Collection::value_type::second_type value = 
std::move(it->second);
Ditto: consider changing Collection::value_type::second_type --> 
Collection::mapped_type for better readability.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I65d17dd26317f77c4c0aa4b8ec00fbe51f8d1ced
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Add a FindPointeeOrNull method to map-util

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add a FindPointeeOrNull method to map-util
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3594/2/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 242: // Returns the raw pointer contained in the smart pointer, if the key 
exists or null if it doesn't.
Does it make sense to add something like 'contained in the smart pointer for 
the first found key'?


Line 244: typename Collection::value_type::second_type::element_type*
Collection::value_type::second_type::element_type --> 
Collection::mapped_type::element_type?


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

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

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 5:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/3593/5/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 792: pair
Why not just MapContainer::mapped_type, but 
MapContainer::value_type::first_type?


Line 793: ComputeIfAbsentReturnAbsense(MapContainer* container,
> it's due to code style. we pass by pointer when we'll be mutating the conta
OK, code style is code style, whether it's ugly or not.

But if using pointers, why there is no check for a nullptr then?  Is this ok to 
get SIGSEGV when calling this function with a null pointer as a parameter or 
there should be some code which checks for null before de-referencing it?


Line 794:  const typename 
MapContainer::value_type::first_type& key,
MapContainer::value_type::first_type --> MapContainer::key_type?


Line 814: typename MapContainer::value_type::second_type* const
MapContainer::mapped_type?


Line 815: ComputeIfAbsent(MapContainer* container,
Does in make sense to reflect the fact the value is being inserted?  I.e., name 
this InsertComputedIfAbsent()


Line 816: const typename MapContainer::value_type::first_type& 
key,
MapContainer::value_type::first_type --> MapContainer::key_type?


http://gerrit.cloudera.org:8080/#/c/3593/5/src/kudu/util/map-util-test.cc
File src/kudu/util/map-util-test.cc:

Line 58: TEST(ComputeIfAbsentTest, TestComputeIfAbsentAntReturnAbsense) {
typo? TestComputeIfAbsentAntReturnAbsense --> 
TestComputeIfAbsentAndReturnAbsense

Or just rename it into TestComputeIfAbsentReturnAbsense


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

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

[kudu-CR] Add a EraseKeyReturnSmartPtrValue to map-util

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add a EraseKeyReturnSmartPtrValue to map-util
..


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/3595/1/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 700: // EraseKeyReturnSmartPtrValue(_map, "my_key, _value);
A tiny typo: "my_key -- > "my_key"


Line 701: template 
What if the collection is a multi-key container, like multimap?  Is it OK just 
to remove the first element having that key and leave the rest?  If so, please 
add explanation about that in the comment for the function.


Line 702: void EraseKeyReturnSmartPtrValue(Collection* const collection,
Consider returning the result as a smart pointer instead of placing it into the 
parameter placeholder.  From the caller's perspective that would be more terse 
then:

unique_ptr p(EraseKeyReturnSmartPtrValue(my_map, "key");

Also, why not to pass input collection by const reference?  Passing parameter 
by pointer means it might be null, so it's necessary to take care of that.


Line 703:  const typename 
Collection::value_type::first_type& key,
Why not just Collection::key_type, but Collection::value_type::first_type?


http://gerrit.cloudera.org:8080/#/c/3595/1/src/kudu/util/map-util-test.cc
File src/kudu/util/map-util-test.cc:

Line 80: TEST(EraseKeyReturnSmartPtrValueTest, TestEraseKeyReturnSmartPtrValue) 
{
Does it make sense to add a testcase for multi-key dictionary like multimap?


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

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

[kudu-CR] Add ComputeIfAbsent methods to map-util

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Add ComputeIfAbsent methods to map-util
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3593/5/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:

Line 780: // it uses the lambda function to create a value, inserts it into the 
map, and returns a a pair with
typo: an extra 'a': '...and returns a a pair...'


Line 791: template 
How costly is compute_func compared with locating the key in the map?  If in 
most use cases compute_func is cheap, then it makes sense just to use stock 
insert() method right away, not trying to locate the element first and not 
calling this method.  But I suppose this is not the case, right?


Line 793: ComputeIfAbsentReturnAbsense(MapContainer* container,
Why not to pass the container by reference here?  Is that due to code style?  
If passing by pointer, what if parameter is null?  Is any check needed here?


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

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

[kudu-CR] Do not run (g)addr2line translator on MacOS X

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Do not run (g)addr2line translator on MacOS X
..


Patch Set 2:

> Thanks for making those changes. Looks good to me, though
 > ultimately I'll defer to someone who actually uses Mac OS.
 > 
 > If you do have some time, I'd love to know whether translation
 > works in sanitizer builds on Mac OS. No worries if you've got other
 > thing to look at, though.

Yep, sure -- I would like to get some feedback from Dan Burkert.  Yes, I'll try 
those sanitizer builds: I'm also curious how that works for those builds.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf8a81ae7f2e1b938734a4b66bec82cb731d90cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] Do not run (g)addr2line translator on MacOS X

[Alexey Serbin (Code Review)]

Hello Mike Percy, Kudu Jenkins,

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

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

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

Change subject: Do not run (g)addr2line translator on MacOS X
..

Do not run (g)addr2line translator on MacOS X

When running tests on MacOS X, omit using the
stacktrace_addr2line.pl utility to translate addresses to line numbers.
For current builds on MacOS X, neither (g)addr2line nor atos
translate stack address into line number without extra-hassle.
This is so even for binaries linked without PIE
(see the -no_pie linker's option).

As for the extra-hassle part: there are at least two ways
to address that, but neither looks like a good candidate
in the context of the script.

  Option A:
Provide the atos utility with reference to running test process
using '-p ' option.  This, however, triggers a pop-up
requesting for password if it's run the very first time.

  Option B:
Take a snapshot of running test process using MacOS X tools
and provide the atos utility with the load address
using '-l ' option.

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


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idf8a81ae7f2e1b938734a4b66bec82cb731d90cc
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] Do not run (g)addr2line translator on MacOS X

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Do not run (g)addr2line translator on MacOS X
..


Patch Set 1:

(2 comments)

> (2 comments)
 > 
 > My understanding is that sanitizer-enabled Kudu builds on Mac OS
 > will report correct line numbers since they use LLVM's built-in
 > symbolizer, so translation is only actually missing on the
 > addresses emitted by a CHECK() or equivalent crash. Is this
 > correct?

Thank you for review!

I haven't tried to run sanitizer-enabled builds.  Yes, as I understand the 
translation is needed for crash-related stacktrace printouts.  Anyways, it 
seems safe to remove addr2line translator since it does no do much on MacOS X 
anyways, just takes some to run.

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

PS1, Line 139: UNAME=$(uname -s)
 :   if [ "$UNAME" = "Darwin" ]; then
> How about using the bash built-in $OSTYPE? I think we typically use that in
Done


Line 143: # into line number.  The atos utility is able to do that only if
> Nit: can you fix this line's formatting a bit? Looks like you can squeeze a
Sure -- actually, I found these lines were too long (more than 80 chars).  I'll 
make them shorter and more uniformly filled.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf8a81ae7f2e1b938734a4b66bec82cb731d90cc
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] Do not run (g)addr2line translator on MacOS X

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: Do not run (g)addr2line translator on MacOS X
..

Do not run (g)addr2line translator on MacOS X

When running tests on MacOS X, omit using the
stacktrace_addr2line.pl utility to translate addresses to line numbers.
For current builds on MacOS X, neither (g)addr2line nor atos
translate stack address into line number without extra-hassle.
This is so even for binaries linked without PIE
(see the -no_pie linker's option).

As for the extra-hassle part: there are at least two ways
to address that, but neither looks a good candidate in the context
of the script.

  Option A:
Provide the atos utility with reference to running test process
using '-p ' option.  This, however, triggers a pop-up
requesting for password if it's run the very first time.

  Option B:
Take a snapshot of running test process using MacOS X tools
and provide the atos utility with the load address
using '-l ' option.

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


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

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