date:20180103

[kudu-CR] KUDU-2249 give the TableRecordReader their own KuduClient to use.

2018-01-03 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8921 )

Change subject: KUDU-2249 give the TableRecordReader their own KuduClient to 
use.
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8921/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
File 
java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java:

http://gerrit.cloudera.org:8080/#/c/8921/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@253
PS1, Line 253:   private KuduClient getKuduClient() {
 :
 : String masterAddresses = conf.get(MASTER_ADDRESSES_KEY);
 : this.operationTimeoutMs = 
conf.getLong(OPERATION_TIMEOUT_MS_KEY,
 : AsyncKuduClient.DEFAULT_OPERATION_TIMEOUT_MS);
 : KuduClient kuduClient = new 
KuduClient.KuduClientBuilder(masterAddresses)
 : .defaultOperationTimeoutMs(operationTimeoutMs)
 : .build();
 : 
KuduTableMapReduceUtil.importCredentialsFromCurrentSubject(kuduClient);
 : return kuduClient;
 :   }
> Hi David,
Thanks for pointing out the discussion.
I'd still like to, at the very least, have a comment, on the commit message 
and/or on the code explaining: i) the rationale, in terms of how many new 
clients we can expect; ii) how this is not going to cause 10s or 100s of new 
clients to spawn, increasing the load on the master iii) or if it is, how there 
is no way to avoid it.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
Gerrit-Change-Number: 8921
Gerrit-PatchSet: 1
Gerrit-Owner: Clemens Valiente 
Gerrit-Reviewer: Clemens Valiente 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 04 Jan 2018 04:16:09 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

2018-01-03 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8692 )

Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
..


Patch Set 4:

(2 comments)

I didn't review sasl_client_transport or the krpc changes.

http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:

http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/hms_client.cc@140
PS4, Line 140: // TODO(dan): check necessary?
Perhaps move this logic (and thus the construction of protocol and client_) 
into Start(), so that you can return a bad Status?

Alternatively, you could make HmsClient use two-phase initialization and add an 
Init() method that can return a bad Status.


http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:

http://gerrit.cloudera.org:8080/#/c/8692/4/src/kudu/hms/mini_hms.h@58
PS4, Line 58:   // Configures the mini HMS to use Kerberos.
Why build this as a setter vs. adding it to the constructor? There's no use 
case for calling it multiple times, right? Or for calling it after Start?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 04 Jan 2018 00:44:27 +
Gerrit-HasComments: Yes

[kudu-CR] [master/tserver] enforce re-replication scheme consistency

2018-01-03 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8901 )

Change subject: [master/tserver] enforce re-replication scheme consistency
..


Patch Set 4:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/consensus/replica_management.proto
File src/kudu/consensus/replica_management.proto:

http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/consensus/replica_management.proto@23
PS4, Line 23: // Message to communicate on the replica management details 
between involved
: // actors.
> "Communicates replica management information between servers."
Done


http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/consensus/replica_management.proto@39
PS4, Line 39: required
> I think this field (like pretty much all proto fields) should be optional,
It's a good call.


http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
File src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1685
PS4, Line 1685: it
> in
Done


http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1686
PS4, Line 1686: make
> makes
Done


http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1714
PS4, Line 1714: // The easiest way to have everything setup is to start the 
cluster with
  :   // compatible parameters.
> Could you move this comment above where the flags are set? I was confused b
Done


http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc@1739
PS4, Line 1739:   SleepFor(MonoDelta::FromMilliseconds(heartbeat_interval_ms * 
3));
> warning: either cast from 'int' to 'int64_t' (aka 'long') is ineffective, o
Done


http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/master/master.proto@80
PS4, Line 80: INCOMPATIBILITY
> Consider "Incompatible".
I think the codes in this enum mean 'the reason for an error', so from that 
perspective 'INCOMPATIBILITY' looks better.


http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/master/master.proto@293
PS4, Line 293: iff
> nit: extra f? Given the identical phrasing below I'm guessing this f is ext
Done


http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/master/master.proto@294
PS4, Line 294: optional consensus.ReplicaManagementInfoPB 
replica_management_info = 7;
> Would it be better to put some kind of repeated feature flag here instead o
I thought about the feature flag, but I decided to go with a field.

The feature flag is to indicate whether some feature is supported/understood by 
the implementation, not whether it's turned on or off.  So, it should be more 
than just one bit for that anyway.

The other reason I opted for a field is that I think we should eventually get 
rid of different replica management schemes.

However, I will give it a second thought.


http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/8901/4/src/kudu/master/master_service.cc@158
PS4, Line 158: what is
> nit: remove
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I71c4c2e72bb2d62cec6de0f6d00b418377e8ae85
Gerrit-Change-Number: 8901
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Thu, 04 Jan 2018 00:44:49 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling

2018-01-03 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8494 )

Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
..


Patch Set 7:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:

http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/hms_client.h@51
PS7, Line 51: the
to


http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/hms_client.h@84
PS7, Line 84: explicit
No longer needed now that this is multi-arg?


http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/hms_client.h@85
PS7, Line 85:  MonoDelta send_timeout = 
MonoDelta::FromSeconds(60),
Would be more ergonomic to create an HmsClientOptions struct and stuff these 
(with documentation) in there.


http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:

http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/mini_hms.cc@111
PS7, Line 111: "-p", std::to_string(port_),
Was this change made so that you could use Start() to restart and have it use 
the same port as before? Might want to document that behavior.

Also, how do we prevent test flakiness caused by another application binding to 
our previously bound ephemeral port?


http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/mini_hms.cc@131
PS7, Line 131: WARN_NOT_OK
Why not RETURN_NOT_OK?


http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/mini_hms.cc@139
PS7, Line 139: KUDU
You can drop this prefix. In Continue() too.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 04 Jan 2018 00:36:29 +
Gerrit-HasComments: Yes

[kudu-CR] Add 'kudu fs list' tool

2018-01-03 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8911 )

Change subject: Add 'kudu fs list' tool
..


Patch Set 6:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8911/6/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/8911/6/src/kudu/tools/kudu-tool-test.cc@404
PS6, Line 404: const vector kFsModeRegexes = {
This should be updated.


http://gerrit.cloudera.org:8080/#/c/8911/6/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:

http://gerrit.cloudera.org:8080/#/c/8911/6/src/kudu/tools/tool_action_fs.cc@396
PS6, Line 396:   switch (group) {
What happens if I add an entry to FieldGroup but forgot to update this switch? 
Do I get a compile error? A warning? Nothing?

If it's not a compile error, is there anything we should add here to guarantee 
good behavior? Like a default statement, a LOG(FATAL), or something like that?


http://gerrit.cloudera.org:8080/#/c/8911/6/src/kudu/tools/tool_action_fs.cc@415
PS6, Line 415:   static const Field kFieldVariants[] = {
Could you use the enum macros from gutil/casts.h to avoid this?


http://gerrit.cloudera.org:8080/#/c/8911/6/src/kudu/tools/tool_action_fs.cc@483
PS6, Line 483: // Returns rowset info for the field.
Worth logging the min/max keys of the rowset?


http://gerrit.cloudera.org:8080/#/c/8911/6/src/kudu/tools/tool_action_fs.cc@584
PS6, Line 584: CHECK_OK(fs_manager->OpenBlock(block, &readable_block));
CHECK_OK seems wrong for a CLI tool; why not RETURN_NOT_OK and return an error 
from List if one of these fails?


http://gerrit.cloudera.org:8080/#/c/8911/6/src/kudu/tools/tool_action_fs.cc@620
PS6, Line 620: tablet_ids.emplace_back(FLAGS_tablet_id);
Do we need to ToLowerCase this?


http://gerrit.cloudera.org:8080/#/c/8911/6/src/kudu/tools/tool_action_fs.cc@633
PS6, Line 633: WARN_NOT_OK(TabletMetadata::Load(&fs_manager, tablet_id, 
&tablet_metadata),
Why not RETURN_NOT_OK on this?


http://gerrit.cloudera.org:8080/#/c/8911/6/src/kudu/tools/tool_action_fs.cc@652
PS6, Line 652:   if (FLAGS_rowset_id > 0 && FLAGS_rowset_id != rowset.id()) 
{
Shouldn't this be FLAGS_rowset_id != -1 && FLAGS_rowset_id != rowset.id() ?


http://gerrit.cloudera.org:8080/#/c/8911/6/src/kudu/tools/tool_action_fs.cc@688
PS6, Line 688:   // TODO(dan): should orphaned blocks be included, perhaps 
behind a flag?
Perhaps, but this comment is slightly misplaced; orphaned blocks are a 
tablet-level thing, not a rowset-level thing, so the comment should be outside 
the inner (rowset_metadata) loop.


http://gerrit.cloudera.org:8080/#/c/8911/6/src/kudu/tools/tool_action_fs.cc@786
PS6, Line 786:"Possible values: table, 
table-id, tablet-id, partition, "
Seems like it might be easy to accidentally omit an entry; could we construct 
this list on-the-fly by iterating on the Field enum class?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f5a63e636d95e3ee55bb4955cece7f5d0b7532d
Gerrit-Change-Number: 8911
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Thu, 04 Jan 2018 00:22:53 +
Gerrit-HasComments: Yes

[kudu-CR] periodic: fix a comment

2018-01-03 Thread Adar Dembo (Code Review)

Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8931 )

Change subject: periodic: fix a comment
..

periodic: fix a comment

Change-Id: Id04bff4f6505595f60f26abe88de58cdded4d9d0
Reviewed-on: http://gerrit.cloudera.org:8080/8931
Reviewed-by: Alexey Serbin 
Tested-by: Kudu Jenkins
---
M src/kudu/rpc/periodic.h
1 file changed, 1 insertion(+), 1 deletion(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id04bff4f6505595f60f26abe88de58cdded4d9d0
Gerrit-Change-Number: 8931
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy

[kudu-CR] periodic: fix a comment

2018-01-03 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8931 )

Change subject: periodic: fix a comment
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id04bff4f6505595f60f26abe88de58cdded4d9d0
Gerrit-Change-Number: 8931
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Wed, 03 Jan 2018 20:09:27 +
Gerrit-HasComments: No

[kudu-CR] periodic: fix a comment

2018-01-03 Thread Adar Dembo (Code Review)

Hello Mike Percy, Alexey Serbin,

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

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

to review the following change.


Change subject: periodic: fix a comment
..

periodic: fix a comment

Change-Id: Id04bff4f6505595f60f26abe88de58cdded4d9d0
---
M src/kudu/rpc/periodic.h
1 file changed, 1 insertion(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id04bff4f6505595f60f26abe88de58cdded4d9d0
Gerrit-Change-Number: 8931
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Mike Percy

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

2018-01-03 Thread Alexey Serbin (Code Review)

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 6:

(12 comments)

It seems I took a quick look at PS1 some time ago.  Will re-visit this week.

http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/client/schema.h
File src/kudu/client/schema.h:

http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/client/schema.h@64
PS5, Line 64: public:
nit: alignment


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h
File src/kudu/client/schema.h:

http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@71
PS1, Line 71: : pre
I don't think this is needed unless we want to protect against list-initialized 
constructors.


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@76
PS1, Line 76:   ret
drop


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@81
PS1, Line 81:   ret
drop


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@321
PS1, Line 321:   ///
Add the documented description for the precision parameter.


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/client/schema.h@328
PS1, Line 328:   ///
Add the documented description for the scale parameter.


http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/common.proto
File src/kudu/common/common.proto:

http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/common.proto@54
PS5, Line 54:   DECIMAL32 = 17;
nit: could you add a comment to explain why 15 and 16 are skipped?


http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/decimal_value.h
File src/kudu/common/decimal_value.h:

http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/decimal_value.h@32
PS5, Line 32:
nit: spacing


http://gerrit.cloudera.org:8080/#/c/8830/5/src/kudu/common/decimal_value.h@43
PS5, Line 43:
nit: by code guidelines, the indent should be 2 spaces.


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

http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@50
PS1, Line 50:  DataType type) const {
nit: misaligned line for the second parameter


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@57
PS1, Line 57: true
Why true, not false?  If there is some specific reason, please add a comment 
about that.


http://gerrit.cloudera.org:8080/#/c/8830/1/src/kudu/common/schema.cc@125
PS1, Line 125:   return strings::Substitute("$0$1 $2",
nit: missed space



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 03 Jan 2018 19:57:40 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

2018-01-03 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 6:

(4 comments)

I only looked at the public API changes.

http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h
File src/kudu/client/schema.h:

http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@64
PS6, Line 64: public:
Nit: indentation


http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@70
PS6, Line 70:   explicit KuduColumnTypeAttributes(int32_t precision, int32_t 
scale)
The 'explicit' keyword is only needed to avoid implicit conversions with 
single-arg constructors, no?


http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@75
PS6, Line 75:   const int32_t precision() const {
These accessors are returning an int32_t 'copy', so what value does the 'const' 
keyword add (as in 'const int32_t ...')?


http://gerrit.cloudera.org:8080/#/c/8830/6/src/kudu/client/schema.h@84
PS6, Line 84:  private:
Do you anticipate KuduColumnTypeAttributes evolving for other use cases in the 
future? If so, you should PIMPL the class: define a private Data nested class, 
store precision/scale within it, and have the public class merely store an 
allocated pointer to the nested class.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 03 Jan 2018 19:18:24 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2216. Post process gtest generated xml to include the output from the *.txt files

2018-01-03 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8757 )

Change subject: KUDU-2216. Post process gtest generated xml to include the 
output from the *.txt files
..


Patch Set 12:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py
File build-support/jenkins/add_std_out_to_junit.py:

http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py@68
PS12, Line 68:  Globs for *.gz ands *. xml logs in log_location, then sorts 
the logs
I'm missing a ton of context, but why not run this script out of run-test.sh 
instead of build-and-test.sh? That way it'll run once per test and the precise 
gz/xml file pair will be known, right?


http://gerrit.cloudera.org:8080/#/c/8757/12/build-support/jenkins/add_std_out_to_junit.py@69
PS12, Line 69:  I
Comment style nit: you'll find the pronouns "we" and "us" more commonly used 
than "I" and "me".



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f4a9a147f29e37e380cb32ff5af43e1290a1a70
Gerrit-Change-Number: 8757
Gerrit-PatchSet: 12
Gerrit-Owner: Edward Fancher 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Edward Fancher 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 03 Jan 2018 19:07:51 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2229. consensus: Leader should not start FD

2018-01-03 Thread Adar Dembo (Code Review)

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8711 )

Change subject: KUDU-2229. consensus: Leader should not start FD
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8711/4/src/kudu/rpc/periodic.h
File src/kudu/rpc/periodic.h:

http://gerrit.cloudera.org:8080/#/c/8711/4/src/kudu/rpc/periodic.h@144
PS4, Line 144:   // Returns true iff the failure detected has been started.
FWIW, this comment is wrong: it refers to failure detection rather than to this 
generic periodic timer system.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2ec9c5499e8e4c1659333bd53dd2d7f6dae81f5
Gerrit-Change-Number: 8711
Gerrit-PatchSet: 4
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Wed, 03 Jan 2018 18:51:52 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2249 give the TableRecordReader their own KuduClient to use.

2018-01-03 Thread Clemens Valiente (Code Review)

Clemens Valiente has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8921 )

Change subject: KUDU-2249 give the TableRecordReader their own KuduClient to 
use.
..


Patch Set 1:

(1 comment)

Hi David,
I discussed this on the dev mailing list here:
http://mail-archives.apache.org/mod_mbox/kudu-dev/201712.mbox/%3CAM0PR0502MB405140DD23048A17522BB55C9C070%40AM0PR0502MB4051.eurprd05.prod.outlook.com%3E

The problem is the getInputSplit() closing the client that the Reader want to 
use.

There's no real clean way of sharing the client and closing it properly. Due to 
the mapreduce architecture, each tablet usually will read from a separate Map 
Container and thus need its own client anyway. This just fixes a bug in the 
rare scenario that the getinputsplit and record reader are executed in one 
container.

http://gerrit.cloudera.org:8080/#/c/8921/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java
File 
java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java:

http://gerrit.cloudera.org:8080/#/c/8921/1/java/kudu-mapreduce/src/main/java/org/apache/kudu/mapreduce/KuduTableInputFormat.java@253
PS1, Line 253:   private KuduClient getKuduClient() {
 :
 : String masterAddresses = conf.get(MASTER_ADDRESSES_KEY);
 : this.operationTimeoutMs = 
conf.getLong(OPERATION_TIMEOUT_MS_KEY,
 : AsyncKuduClient.DEFAULT_OPERATION_TIMEOUT_MS);
 : KuduClient kuduClient = new 
KuduClient.KuduClientBuilder(masterAddresses)
 : .defaultOperationTimeoutMs(operationTimeoutMs)
 : .build();
 : 
KuduTableMapReduceUtil.importCredentialsFromCurrentSubject(kuduClient);
 : return kuduClient;
 :   }
> Kudu clients cache state internally, like tablet locations. Changing this t
Hi David,
I discussed this on the dev mailing list here:
http://mail-archives.apache.org/mod_mbox/kudu-dev/201712.mbox/%3CAM0PR0502MB405140DD23048A17522BB55C9C070%40AM0PR0502MB4051.eurprd05.prod.outlook.com%3E

The problem is the getInputSplit() closing the client that the Reader want to 
use.

There's no real clean way of sharing the client and closing it properly. Due to 
the mapreduce architecture, each tablet usually will read from a separate Map 
Container and thus need its own client anyway. This just fixes a bug in the 
rare scenario that the getinputsplit and record reader are executed in one 
container.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24f45ee9253790c5348cabd0afe6c6a4b6d3f3d4
Gerrit-Change-Number: 8921
Gerrit-PatchSet: 1
Gerrit-Owner: Clemens Valiente 
Gerrit-Reviewer: Clemens Valiente 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 03 Jan 2018 08:39:45 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2249 give the TableRecordReader their own KuduClient to use.

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[kudu-CR] [master/tserver] enforce re-replication scheme consistency

[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling

[kudu-CR] Add 'kudu fs list' tool

[kudu-CR] periodic: fix a comment

[kudu-CR] periodic: fix a comment

[kudu-CR] periodic: fix a comment

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[kudu-CR] KUDU-2216. Post process gtest generated xml to include the output from the *.txt files

[kudu-CR] KUDU-2229. consensus: Leader should not start FD

[kudu-CR] KUDU-2249 give the TableRecordReader their own KuduClient to use.

13 matches

Site Navigation

Mail list logo

Footer information