[kudu-CR] Memory tracking for result tracker

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Memory tracking for result tracker
..

Memory tracking for result tracker

This adds memory tracking to ResultTracker, making sure we account for the 
memory
as we cache responses for clients' requests.

Testing wise this adds memory consumption checks to rpc-stress-test.cc.

Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/server/server_base.cc
5 files changed, 182 insertions(+), 16 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Memory tracking for result tracker

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 15:

Build Started http://104.196.14.100/job/kudu-gerrit/2511/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Memory tracking for result tracker

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 13:

(9 comments)

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

PS13, Line 78:   // Release all the memory for the stuff we'll delete on 
destruction.
 :   for (auto& client_state : clients_) {
 : for (auto& completion_record : 
client_state.second->completion_records) {
 :   
mem_tracker_->Release(completion_record.second->memory_footprint());
 : }
 : 
mem_tracker_->Release(client_state.second->memory_footprint());
 :   }
 :   mem_tracker_->Release(memory_footprint());
> Can we aggregate this and make just one Release() call? This isn't a hot pa
yeah this isn't a hot path at all, and I found it helpful for debugging to have 
separate releases that you can selectively comment out.


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

Line 228: return client_state_map_bytes_;
> Typically memory_footprint() methods are supposed to account for the entire
well the CompletionREcord's memory calculation is not shallow. So it'd be wrong 
in that case. I'd rather keep the naming and add some comments.


Line 254: int64_t memory_footprint() const {
> Likewise, memory_footprint_excluding_rpcs.
Seem my comment above


Line 255:   return kudu_malloc_usable_size(this)
> Does this method need to be synchronized in some way? Can the CompletionRec
these structs are assumed to be mutated under the lock. I think it makes sense 
to have Unlocked/Locked when there's a mix of locking in methods but this is 
not the case


Line 281: int64_t memory_footprint() const {
> Likewise, memory_footprint_excluding_completion_records.
See my comment above.


Line 282:   return kudu_malloc_usable_size(this) + 
completion_record_map_bytes_;
> Do we need to synchronize access to completion_record_map_bytes_?
same as above


PS13, Line 327:   // Lock that protects access to 'clients_' and to the state 
contained in each
  :   // ClientState.
  :   // TODO consider a per-ClientState lock if we find this too 
coarse grained.
> Does this also protect access to client_state_map_bytes_? If so, should it 
memory_footprint() is only called under the lock.


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

Line 259: usleep(100 * 1000);
> Nit: Can we use SleepFor() instead? It's more idiomatic for Kudu, and makes
Done. We sleep here because the client gets the answer before the memtracker is 
updated


http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

Line 102:   result_tracker_(new 
rpc::ResultTracker(CreateMemTrackerForForResultTracker(mem_tracker_))),
> How about:
actually removed the method above and called CreateTracker inline.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Memory tracking for result tracker

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 14:

Build Started http://104.196.14.100/job/kudu-gerrit/2510/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Memory tracking for result tracker

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Memory tracking for result tracker
..

Memory tracking for result tracker

This adds memory tracking to ResultTracker, making sure we account for the 
memory
as we cache responses for clients' requests.

Testing wise this adds memory consumption checks to rpc-stress-test.cc.

Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/server/server_base.cc
5 files changed, 182 insertions(+), 16 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [java client] Integrate with the replay cache

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] Integrate with the replay cache
..


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3631/4/java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java
File java/kudu-client/src/main/java/org/kududb/client/KuduRpc.java:

PS4, Line 107: long
> docs
Done


http://gerrit.cloudera.org:8080/#/c/3631/4/java/kudu-client/src/main/java/org/kududb/client/Operation.java
File java/kudu-client/src/main/java/org/kududb/client/Operation.java:

Line 163:   boolean isRetryableRpc() {
> docs
You meant to put that in KuduRpc? Or are you saying that this override here 
should be documented?


http://gerrit.cloudera.org:8080/#/c/3631/4/java/kudu-client/src/main/java/org/kududb/client/RequestTracker.java
File java/kudu-client/src/main/java/org/kududb/client/RequestTracker.java:

Line 41:   public RequestTracker(String clientId) {
> does the request tracker need to have the client id?
No, but you seemed keen on having the Java client be like the C++ side, so I 
put it here. Originally I just had it in AsyncKuduClient.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I108cd30acbc308bfb4577d072c2a8f26d1553c68
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] WIP: Add garbage collection to ResultTracker

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: WIP: Add garbage collection to ResultTracker
..


Patch Set 5:

(1 comment)

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

PS5, Line 91: // If the arriving request is older than our per-client GC 
watermark, report its
:   // staleness to the client.
> coming back as an RPC error (we don't cache results for individual rows, on
ok, makes sense


-- 
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-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

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 8:

(6 comments)

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_EXECUTABLE is sufficient, not sure.


Line 989: add_custom_target(doxy_install_client_alt_destdir
What if doxy_install_client_alt_destdir depended on 'all'? Then could you drop 
the explicit "make all" step? I think that would be better, as it'd feed more 
dependency information into the generated makefiles ("make all install" as a 
command doesn't do that). Also, I don't think you'd need the remove_directory 
commands then either.


Line 990:   COMMAND ${CMAKE_COMMAND} -E remove_directory 
${DOXY_CLIENT_DESTDIR}
Why do we need to remove the old directory first?


Line 992:   WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/src/kudu/client
Why does the working directory need to be modified for this command?


Line 997:   COMMAND ${CMAKE_COMMAND} -E remove_directory 
${DOXY_CLIENT_DESTDIR}
Why 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 to 
me, but maybe I'm missing something.


-- 
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] Memory tracking for result tracker

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Memory tracking for result tracker
..

Memory tracking for result tracker

This adds memory tracking to ResultTracker, making sure we account for the 
memory
as we cache responses for clients' requests.

Testing wise this adds memory consumption checks to rpc-stress-test.cc.

Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/server/server_base.cc
5 files changed, 182 insertions(+), 16 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Memory tracking for result tracker

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 12:

Build Started http://104.196.14.100/job/kudu-gerrit/2507/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Memory tracking for result tracker

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Memory tracking for result tracker
..

Memory tracking for result tracker

This adds memory tracking to ResultTracker, making sure we account for the 
memory
as we cache responses for clients' requests.

Testing wise this adds memory consumption checks to rpc-stress-test.cc.

Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/server/server_base.cc
5 files changed, 183 insertions(+), 16 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Memory tracking for result tracker

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 11:

Build Started http://104.196.14.100/job/kudu-gerrit/2506/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Memory tracking for result tracker

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Memory tracking for result tracker
..

Memory tracking for result tracker

This adds memory tracking to ResultTracker, making sure we account for the 
memory
as we cache responses for clients' requests.

Testing wise this adds memory consumption checks to rpc-stress-test.cc.

Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/server/server_base.cc
5 files changed, 178 insertions(+), 16 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Memory tracking for result tracker

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Memory tracking for result tracker
..

Memory tracking for result tracker

This adds memory tracking to ResultTracker, making sure we account for the 
memory
as we cache responses for clients' requests.

Testing wise this adds memory consumption checks to rpc-stress-test.cc.

Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/server/server_base.cc
5 files changed, 178 insertions(+), 16 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Memory tracking for result tracker

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 13:

Build Started http://104.196.14.100/job/kudu-gerrit/2508/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[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] Memory tracking for result tracker

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 10:

Build Started http://104.196.14.100/job/kudu-gerrit/2505/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3501/5/src/kudu/consensus/consensus_queue-test.cc
File src/kudu/consensus/consensus_queue-test.cc:

Line 851:   // Committed index should be the same.
> This patch doesn't change how followers handle the committed index. It chan
I agree that since the committed index is sent to the followers by the leader, 
in theory we should be able to support it as a metric on the followers. I 
haven't really tried to dig into this but if we're not just fixing that problem 
then I would like to understand why not.


http://gerrit.cloudera.org:8080/#/c/3501/6/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

Line 1774:   out << "" << EscapeForHtmlToString(state_->ToString()) << 
"" << std::endl;
This is a good thing to add but I think it wouldn't hurt to have both here. 
Something like:

 out << ""
 << EscapeForHtmlToString(state_->ToString())
 << EscapeForHtmlToString(queue_->ToString())
 << "" << std::endl;


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-763 consensus queue metrics on followers are messed up

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: KUDU-763 consensus queue metrics on followers are messed up
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3501/6/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

Line 1774:   out << "" << EscapeForHtmlToString(state_->ToString()) << 
"" << std::endl;
> This is a good thing to add but I think it wouldn't hurt to have both here.
Also, maybe this change should be included in the other patch with the web UI 
changes


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fb0d45f85786b9e2631b5dc0bf044a9d3192a39
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

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

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

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


Patch Set 8:

(2 comments)

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

Line 989: add_custom_target(doxy_install_client_alt_destdir
> Yesterday I tried dependency on 'kudu_client_exported' as well :)  The prob
Interesting. I think the kudu_client_exported problem is due a misuse of 
add_dependencies(), where the gen_version_info target is set as a dependency of 
kudu_util but not kudu_util_exported.

Can you try this patch? It should allow depending on kudu_client_exported 
properly, and it's a good improvement to make regardless.

  diff --git a/src/kudu/util/CMakeLists.txt b/src/kudu/util/CMakeLists.txt
  index a924f38..df1b4c9 100644
  --- a/src/kudu/util/CMakeLists.txt
  +++ b/src/kudu/util/CMakeLists.txt
  @@ -218,10 +218,9 @@ endif()
   ADD_EXPORTABLE_LIBRARY(kudu_util
 SRCS ${UTIL_SRCS}
 DEPS ${UTIL_LIBS}
  +  NONLINK_DEPS gen_version_info
 EXPORTED_DEPS ${EXPORTED_UTIL_LIBS})
 
  -add_dependencies(kudu_util gen_version_info)
  -
   ###
   # kudu_test_util
   ###


Line 992:   WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/src/kudu/client
> Yes, but why to depend on that at all and spend extra-time fixing that if s
Running make out of anywhere but the top-level directory isn't something most 
(if any) of us do, so I'm not sure how much I trust it. Plus, it means another 
place to update if we wanted to move the client code (or the client's install 
code) to a different directory. In that sense, the target abstracts away the 
filesystem layout, and that's a good thing.


-- 
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] Add port in web ui tables, add role and table name to /tablet page

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: Add port in web ui tables, add role and table name to /tablet 
page
..


Add port in web ui tables, add role and table name to /tablet page

Couple of improvements to web ui:
1. The RaftConfig column in the tablet table on master and tserver now
displays the port as well as the hostname for the tablet.
2. The /tablet page displays the tablet role and the table name.

Change-Id: Ia80b74346cae1a75d66b400521e07aa1994d1d65
Reviewed-on: http://gerrit.cloudera.org:8080/3553
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans 
---
M src/kudu/master/master-path-handlers.cc
M src/kudu/tserver/tserver-path-handlers.cc
2 files changed, 11 insertions(+), 4 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia80b74346cae1a75d66b400521e07aa1994d1d65
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Memory tracking for result tracker

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 13:

(10 comments)

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

PS13, Line 78:   // Release all the memory for the stuff we'll delete on 
destruction.
 :   for (auto& client_state : clients_) {
 : for (auto& completion_record : 
client_state.second->completion_records) {
 :   
mem_tracker_->Release(completion_record.second->memory_footprint());
 : }
 : 
mem_tracker_->Release(client_state.second->memory_footprint());
 :   }
 :   mem_tracker_->Release(memory_footprint());
Can we aggregate this and make just one Release() call? This isn't a hot path, 
but it seems easy enough to do.


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

Line 228: return client_state_map_bytes_;
Typically memory_footprint() methods are supposed to account for the entirety 
of the object, including any nested allocations. Since you're just using it for 
a "shallow" accounting, perhaps call it memory_footprint_excluding_clients, and 
add a comment explaining that?

Ahh, the issue is that the ScopedMemTrackerUpdater expects a method named 
memory_footprint(). How about renaming all of them to 
memory_footprint_shallow() instead, and making it clear via comments which 
"deep" structures they are not accounting for?


Line 254: int64_t memory_footprint() const {
Likewise, memory_footprint_excluding_rpcs.


Line 255:   return kudu_malloc_usable_size(this)
Does this method need to be synchronized in some way? Can the CompletionRecord 
be modified while we're executing here? Or is every access expected to hold 
lock_? If so, maybe add Unlocked() to the name of the method.


Line 281: int64_t memory_footprint() const {
Likewise, memory_footprint_excluding_completion_records.


Line 282:   return kudu_malloc_usable_size(this) + 
completion_record_map_bytes_;
Do we need to synchronize access to completion_record_map_bytes_?


PS13, Line 327:   // Lock that protects access to 'clients_' and to the state 
contained in each
  :   // ClientState.
  :   // TODO consider a per-ClientState lock if we find this too 
coarse grained.
Does this also protect access to client_state_map_bytes_? If so, should it be 
taken in memory_footprint()?


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

Line 259: usleep(100 * 1000);
Nit: Can we use SleepFor() instead? It's more idiomatic for Kudu, and makes the 
units a little clearer.

Separately, why do we need to sleep here?


http://gerrit.cloudera.org:8080/#/c/3627/9/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

PS9, Line 89: }
: 
: } /
> Done re the id.
Hmm, OK, I guess that's true. Admittedly a separate memtracker is more of an 
issue when its parent is the tablet memtracker, as then a server can have 
hundreds or thousands additional memtrackers, which muddies the memory UI view 
significantly.


http://gerrit.cloudera.org:8080/#/c/3627/13/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

Line 102:   result_tracker_(new 
rpc::ResultTracker(CreateMemTrackerForForResultTracker(mem_tracker_))),
How about:

  result_tracker_(new rpc::ResultTracker(MemTracker::CreateTracker(-1, 
"result-tracker", mem_tracker_))),

Or is that too long?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1487 Add instructions for cutting a release

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged.

Change subject: KUDU-1487 Add instructions for cutting a release
..


KUDU-1487 Add instructions for cutting a release

Change-Id: I5b52edb68d35d07ee50bb3c373bf866560f5bc93
Reviewed-on: http://gerrit.cloudera.org:8080/3614
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy 
---
A RELEASING.adoc
1 file changed, 195 insertions(+), 0 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I5b52edb68d35d07ee50bb3c373bf866560f5bc93
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Misty Stanley-Jones 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 

[kudu-CR] Make block manager-test work on OS X again

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Make block_manager-test work on OS X again
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3636/2/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:

Line 85: CHECK_OK(bm_->Create());
How about just add a RETURN_NOT_LOG_BLOCK_MANAGER() here at the top of SetUp()?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56af74ad6b973c90057680be719b257109924fca
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

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

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

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


Patch Set 9:

Build Started http://104.196.14.100/job/kudu-gerrit/2509/

-- 
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: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[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)]

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