[kudu-CR] KUDU-2279 (part 5): don't include entity attributes in log

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-2279 (part 5): don't include entity attributes in log
..

KUDU-2279 (part 5): don't include entity attributes in log

Entity attributes like table names and partition descriptions aren't
very useful in the context of the metrics log. They can always be
grabbed from other places like 'kudu remote_replica list', etc, and are
rarely relevant for tserver-scope debugging.

Additionally, these might be considered somewhat sensitive for users to
share publically in bug reports, etc, so may as well "redact" them.

This shaves a few more bytes off the entries in the metrics log.

Change-Id: Ic995f6a3b8430b73a041de8e5bcbc53a5d8e7f1b
---
M src/kudu/server/server_base.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
3 files changed, 15 insertions(+), 6 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic995f6a3b8430b73a041de8e5bcbc53a5d8e7f1b
Gerrit-Change-Number: 9180
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2279 (part 3): metrics: don't emit untouched metrics to log

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-2279 (part 3): metrics: don't emit untouched metrics to log
..

KUDU-2279 (part 3): metrics: don't emit untouched metrics to log

This avoids a big dump of zero-valued metrics at startup.

Change-Id: I92d2c4640e54c91791fab9c420215bafa3fe8f20
---
M src/kudu/server/server_base.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
4 files changed, 68 insertions(+), 3 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I92d2c4640e54c91791fab9c420215bafa3fe8f20
Gerrit-Change-Number: 9177
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-2279 (part 2): metrics: only emit changed metrics in 
metrics log
..

KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log

This adds a global 'metrics epoch' which can be externally incremented.
When metrics are modified, they remember the epoch of their most recent
modification.

When we dump metrics, we can pass a lower bound in order to see only
metrics which have been modified in or after a given epoch.

This patch updates the metrics logging to only emit metrics that have
changed in each successive line. This should substantially reduce the
size and CPU cost of metric logging on servers with thousands of
tablets.

Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
---
M src/kudu/server/server_base.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
4 files changed, 141 insertions(+), 26 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
Gerrit-Change-Number: 9176
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] jsonwriter: small optimization for PB fields

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9184 )

Change subject: jsonwriter: small optimization for PB fields
..

jsonwriter: small optimization for PB fields

This avoids extra non-inlinable calls into the protobuf library:
- extra calls to GetReflection()
- calls to FieldSize() while looping through repeated fields

This has a small benefit on all PBs and a big benefit on those PBs with
a lot of repeated fields. The latter should be a nice boost particularly
when serializing metrics.

Before:
TestJsonWriter.BenchmarkAllTypes  Throughput: 157.609MB/sec
TestJsonWriter.BenchmarkNestedMessage Throughput: 124.048MB/sec
TestJsonWriter.BenchmarkRepeatedInt64 Throughput: 113.77MB/sec

After:
TestJsonWriter.BenchmarkAllTypes  Throughput: 163.327MB/sec  (+3.6%)
TestJsonWriter.BenchmarkNestedMessage Throughput: 127.633MB/sec  (+2.9%)
TestJsonWriter.BenchmarkRepeatedInt64 Throughput: 158.229MB/sec  (+39.1%)

Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Reviewed-on: http://gerrit.cloudera.org:8080/9184
Reviewed-by: Sailesh Mukil 
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/jsonwriter.cc
M src/kudu/util/jsonwriter.h
3 files changed, 36 insertions(+), 11 deletions(-)

Approvals:
  Sailesh Mukil: Looks good to me, but someone else must approve
  Kudu Jenkins: Verified
  Todd Lipcon: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] jsonwriter: small optimization for PB fields

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9184 )

Change subject: jsonwriter: small optimization for PB fields
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Sat, 03 Feb 2018 01:53:01 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9176 )

Change subject: KUDU-2279 (part 2): metrics: only emit changed metrics in 
metrics log
..


Patch Set 2:

(7 comments)

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

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/server/server_base.cc@630
PS2, Line 630: opts.only_modified_since_epoch = prev_log_epoch + 1;
 : int64_t this_log_epoch = Metric::current_epoch();
 : Metric::IncrementEpoch();
> the reason for the 'prev_log_epoch' thing is that, in the odd case that we
Done


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

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics-test.cc@332
PS2, Line 332: epoch_before_modify
> This is the epoch of/containing the modification, not the epoch before.
Done


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics-test.cc@347
PS2, Line 347: epoch_before_modify
> This should be `new_epoch`.
Done


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h@567
PS2, Line 567: it's
> Remove.
Done


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h@570
PS2, Line 570: base::subtle::NoBarrier_Load(_epoch_)
> will see if I can switch. I get nervous abotu the C++11 atomics because I d
Done


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.cc@357
PS2, Line 357: Metric::IncrementEpoch();
> hm, yea I think this can just be removed. I think it's a holdover from a pr
Done


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.cc@499
PS2, Line 499: 1
> will see if I can do that.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
Gerrit-Change-Number: 9176
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Sat, 03 Feb 2018 01:49:40 +
Gerrit-HasComments: Yes

[kudu-CR] jsonwriter: small optimization for PB fields

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9184 )

Change subject: jsonwriter: small optimization for PB fields
..


Patch Set 2: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Sat, 03 Feb 2018 01:23:39 +
Gerrit-HasComments: No

[kudu-CR] jsonwriter: small optimization for PB fields

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Kudu Jenkins, Sailesh Mukil,

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

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

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

Change subject: jsonwriter: small optimization for PB fields
..

jsonwriter: small optimization for PB fields

This avoids extra non-inlinable calls into the protobuf library:
- extra calls to GetReflection()
- calls to FieldSize() while looping through repeated fields

This has a small benefit on all PBs and a big benefit on those PBs with
a lot of repeated fields. The latter should be a nice boost particularly
when serializing metrics.

Before:
TestJsonWriter.BenchmarkAllTypes  Throughput: 157.609MB/sec
TestJsonWriter.BenchmarkNestedMessage Throughput: 124.048MB/sec
TestJsonWriter.BenchmarkRepeatedInt64 Throughput: 113.77MB/sec

After:
TestJsonWriter.BenchmarkAllTypes  Throughput: 163.327MB/sec  (+3.6%)
TestJsonWriter.BenchmarkNestedMessage Throughput: 127.633MB/sec  (+2.9%)
TestJsonWriter.BenchmarkRepeatedInt64 Throughput: 158.229MB/sec  (+39.1%)

Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
---
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/jsonwriter.cc
M src/kudu/util/jsonwriter.h
3 files changed, 36 insertions(+), 11 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] jsonwriter: do some internal buffering in front of ostringstream

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9181 )

Change subject: jsonwriter: do some internal buffering in front of ostringstream
..

jsonwriter: do some internal buffering in front of ostringstream

JsonWriter outputs into a std::ostringstream, and the RapidJSON
interface is such that we always append one character at a time.
ostringstream::put(char) is not inlined, so this is very slow.

This patch adds a small buffer in front of the ostringstream. We flush
the buffer on the end of any JSON array or object.

The patch adds a simple benchmark to show the improvement:

Before:
I0131 20:26:09.437477 16201 jsonwriter-test.cc:187] Throughput: 87.0644MB/sec

After:
I0131 20:24:17.889117 15929 jsonwriter-test.cc:187] Throughput: 154.192MB/sec

The remaining perf issues seem to be due to our use of protobuf
reflection.

Change-Id: Ib45d58187cf80ecfe2f5a25e40d7042e4a27619d
Reviewed-on: http://gerrit.cloudera.org:8080/9181
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley 
---
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/jsonwriter.cc
2 files changed, 77 insertions(+), 21 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Will Berkeley: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib45d58187cf80ecfe2f5a25e40d7042e4a27619d
Gerrit-Change-Number: 9181
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] metrics: fast-path PB/JSON dump for empty histograms

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9179 )

Change subject: metrics: fast-path PB/JSON dump for empty histograms
..

metrics: fast-path PB/JSON dump for empty histograms

It turns out that a decent percentage of our histograms are empty in
real life use cases. For example, some tablets might be cold enough that
they have never had any action since the tserver started. Other
histograms might correspond to features which aren't being used in a
particular workload (eg commit-wait counters).

For example, in a test cluster running a cycling YCSB workload, about
20% of histograms are empty. In another test cluster running more of an
analytic workload with a lot of very cold tablets, 81% of histograms
were empty.

We can fast-path various calculations on these histograms to just return
0 for a simple speedup on the /metricsjson endpoint.

Change-Id: I0732e8299e2f27c7c6021ec70740984ab46a69c0
Reviewed-on: http://gerrit.cloudera.org:8080/9179
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley 
---
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
2 files changed, 37 insertions(+), 21 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Will Berkeley: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0732e8299e2f27c7c6021ec70740984ab46a69c0
Gerrit-Change-Number: 9179
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2279 (part 3): metrics: don't emit untouched metrics to log

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9177 )

Change subject: KUDU-2279 (part 3): metrics: don't emit untouched metrics to log
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9177/1/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:

http://gerrit.cloudera.org:8080/#/c/9177/1/src/kudu/util/metrics.h@434
PS1, Line 434: gauges may be non-zero and then reset to zero
> aren't there counters we increment by negative amounts
nope, that's not allowed/encouraged -- CM at least assumes that if a counter 
goes down, it means it restarted, and adds the new value to the previously-seen 
value.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I92d2c4640e54c91791fab9c420215bafa3fe8f20
Gerrit-Change-Number: 9177
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Sat, 03 Feb 2018 01:08:05 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2279 (part 1). rolling log: respect logfile retention flag

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9175 )

Change subject: KUDU-2279 (part 1). rolling_log: respect logfile retention flag
..

KUDU-2279 (part 1). rolling_log: respect logfile retention flag

Previously we never deleted old metrics logs, making it somewhat
dangerous to enable this in production. Now we respect the same
retention gflag that we apply to our other glogs.

Change-Id: If3cc9b085c9827fd544b0bf7d7ae868e3692296b
Reviewed-on: http://gerrit.cloudera.org:8080/9175
Tested-by: Kudu Jenkins
Reviewed-by: Will Berkeley 
---
M src/kudu/util/rolling_log-test.cc
M src/kudu/util/rolling_log.cc
M src/kudu/util/rolling_log.h
3 files changed, 83 insertions(+), 39 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Will Berkeley: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If3cc9b085c9827fd544b0bf7d7ae868e3692296b
Gerrit-Change-Number: 9175
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] jsonwriter: small optimization for repeated PB fields

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9184 )

Change subject: jsonwriter: small optimization for repeated PB fields
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9184/1/src/kudu/util/jsonwriter.cc
File src/kudu/util/jsonwriter.cc:

http://gerrit.cloudera.org:8080/#/c/9184/1/src/kudu/util/jsonwriter.cc@197
PS1, Line 197: ProtobufField(pb, field);
> Why not do the same here? I'm assuming pb.GetReflection() is non-inline-abl
good point, will do.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Sat, 03 Feb 2018 00:54:37 +
Gerrit-HasComments: Yes

[kudu-CR] [docs] Added recommendation to compress PK for backfill inserts

[Alex Rodoni (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: [docs] Added recommendation to compress PK for backfill inserts
..

[docs] Added recommendation to compress PK for backfill inserts

Change-Id: I698d954265b4171e4d1bb7e01e286d0d489f1ec7
---
M docs/schema_design.adoc
1 file changed, 64 insertions(+), 18 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I698d954265b4171e4d1bb7e01e286d0d489f1ec7
Gerrit-Change-Number: 9185
Gerrit-PatchSet: 4
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [docs] Added recommendation to compress PK for backfill inserts

[Alex Rodoni (Code Review)]

Alex Rodoni has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9185 )

Change subject: [docs] Added recommendation to compress PK for backfill inserts
..


Patch Set 4:

Fixed a typo in the Backfill section heading.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I698d954265b4171e4d1bb7e01e286d0d489f1ec7
Gerrit-Change-Number: 9185
Gerrit-PatchSet: 4
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 03 Feb 2018 00:46:00 +
Gerrit-HasComments: No

[kudu-CR] jsonwriter: small optimization for repeated PB fields

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9184 )

Change subject: jsonwriter: small optimization for repeated PB fields
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9184/1/src/kudu/util/jsonwriter.cc
File src/kudu/util/jsonwriter.cc:

http://gerrit.cloudera.org:8080/#/c/9184/1/src/kudu/util/jsonwriter.cc@197
PS1, Line 197: ProtobufField(pb, field);
Why not do the same here? I'm assuming pb.GetReflection() is non-inline-able as 
well?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 23:34:14 +
Gerrit-HasComments: Yes

[kudu-CR] Improved logging of DMS flushes

[Will Berkeley (Code Review)]

Will Berkeley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9204


Change subject: Improved logging of DMS flushes
..

Improved logging of DMS flushes

>From compaction-test.cc's TestRowSetInput,

Before:
I0202 14:52:24.983525 2427953984 delta_tracker.cc:727] T test_tablet_id P 
a3e9bdf25f0b431ea57d0249535c29b3: Flushing 20 deltas from DMS 0...
I0202 14:52:24.984046 2427953984 delta_tracker.cc:668] T test_tablet_id P 
a3e9bdf25f0b431ea57d0249535c29b3: Flushed delta block: 0198399763226209 ts 
range: [11, 30]

After:
I0202 14:45:18.924826 2427953984 delta_tracker.cc:726] T test_tablet_id P 
37440651650a4d858267ad408b7e1ac0: Flushing 20 deltas (1759 bytes) from DMS 0
I0202 14:45:18.925750 2427953984 delta_tracker.cc:668] T test_tablet_id P 
37440651650a4d858267ad408b7e1ac0: Flushed delta block 0052242700095437 (296 
bytes) stats: ts range=[11, 30], delete_count=[0], reinsert_count=[0], 
update_counts_by_col_id=[12:20,11:20]

Plus one tiny change to clarify logging on flushes.

Change-Id: I8241a6ae33c22838504aa7007463d11167a22e65
---
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/tablet.cc
3 files changed, 10 insertions(+), 6 deletions(-)



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

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

[kudu-CR](gh-pages) Fix broken doc guide link

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9205 )

Change subject: Fix broken doc guide link
..


Patch Set 1: Verified+1 Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: comment
Gerrit-Change-Id: I6add29ad020516809bb99ba1ce1f1cf374738ba6
Gerrit-Change-Number: 9205
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Comment-Date: Fri, 02 Feb 2018 23:05:23 +
Gerrit-HasComments: No

[kudu-CR](gh-pages) Fix broken doc guide link

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9205 )

Change subject: Fix broken doc guide link
..

Fix broken doc guide link

Change-Id: I6add29ad020516809bb99ba1ce1f1cf374738ba6
Reviewed-on: http://gerrit.cloudera.org:8080/9205
Reviewed-by: Jean-Daniel Cryans 
Tested-by: Jean-Daniel Cryans 
---
M community.md
1 file changed, 1 insertion(+), 1 deletion(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: merged
Gerrit-Change-Id: I6add29ad020516809bb99ba1ce1f1cf374738ba6
Gerrit-Change-Number: 9205
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 

[kudu-CR](gh-pages) Fix broken doc guide link

[Grant Henke (Code Review)]

Grant Henke has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9205


Change subject: Fix broken doc guide link
..

Fix broken doc guide link

Change-Id: I6add29ad020516809bb99ba1ce1f1cf374738ba6
---
M community.md
1 file changed, 1 insertion(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6add29ad020516809bb99ba1ce1f1cf374738ba6
Gerrit-Change-Number: 9205
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke 

[kudu-CR](gh-pages) Update the latest committers

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9203 )

Change subject: Update the latest committers
..


Patch Set 3: Verified+1


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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: comment
Gerrit-Change-Id: I13012ea18ae85bcc61a300a6842d2e35f4a7813d
Gerrit-Change-Number: 9203
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 23:00:17 +
Gerrit-HasComments: No

[kudu-CR](gh-pages) Update the latest committers

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9203 )

Change subject: Update the latest committers
..

Update the latest committers

Change-Id: I13012ea18ae85bcc61a300a6842d2e35f4a7813d
Reviewed-on: http://gerrit.cloudera.org:8080/9203
Reviewed-by: Jean-Daniel Cryans 
Tested-by: Jean-Daniel Cryans 
---
M committers.md
1 file changed, 3 insertions(+), 0 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: merged
Gerrit-Change-Id: I13012ea18ae85bcc61a300a6842d2e35f4a7813d
Gerrit-Change-Number: 9203
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR](gh-pages) Update the latest committers

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9203 )

Change subject: Update the latest committers
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: comment
Gerrit-Change-Id: I13012ea18ae85bcc61a300a6842d2e35f4a7813d
Gerrit-Change-Number: 9203
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 23:00:08 +
Gerrit-HasComments: No

[kudu-CR](gh-pages) Update the latest committers

[Grant Henke (Code Review)]

Hello Todd Lipcon,

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

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

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

Change subject: Update the latest committers
..

Update the latest committers

Change-Id: I13012ea18ae85bcc61a300a6842d2e35f4a7813d
---
M committers.md
1 file changed, 3 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I13012ea18ae85bcc61a300a6842d2e35f4a7813d
Gerrit-Change-Number: 9203
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR](gh-pages) Update the latest committers

[Grant Henke (Code Review)]

Grant Henke has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/9203 )

Change subject: Update the latest committers
..

Update the latest committers

Change-Id: I13012ea18ae85bcc61a300a6842d2e35f4a7813d
---
M committers.md
1 file changed, 4 insertions(+), 1 deletion(-)


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

Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I13012ea18ae85bcc61a300a6842d2e35f4a7813d
Gerrit-Change-Number: 9203
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 

[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8804 )

Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto
File src/kudu/common/common.proto:

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@237
PS10, Line 237:   READ_YOUR_WRITES = 3;
> It may be good to add docs about whether a snapshot timestamp and propagate
Will update.

We do not return the chosen snapshot timestamp, because as we discussed 
offline, for multi tablet scan we should not reuse the timestamps across 
servers in this mode. Each server should be free to choose a timestamp as long 
as it respects the aforementioned condition (This is documented in the design 
doc as well :)). The returned propagated timestamp is set to 'Now' of the 
tserver (same as other scan mode). However, as discussed the client should only 
update the propagated timestamp after the scan of last tablet for multi tablet 
scan.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Fri, 02 Feb 2018 22:49:01 +
Gerrit-HasComments: Yes

[kudu-CR] Fix log message when there's too few data dirs for a data dir group

[Andrew Wong (Code Review)]

Andrew Wong has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9194 )

Change subject: Fix log message when there's too few data dirs for a data dir 
group
..

Fix log message when there's too few data dirs for a data dir group

The address of the dirs full and dirs failed metrics were being
logged instead of the metric value:

I0201 14:45:22.566895  3316 data_dirs.cc:930] Could only allocate 1 dirs of 
requested 3 for tablet 78a4a8db9829474e84b7a5474c9114fd. 1 dirs total, 
0x379c750 dirs full, 0x37599b0 dirs failed

Change-Id: I877d335a75625a087e4e9027b7f311fc8c0805c9
Reviewed-on: http://gerrit.cloudera.org:8080/9194
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong 
---
M src/kudu/fs/data_dirs.cc
1 file changed, 4 insertions(+), 3 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Andrew Wong: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I877d335a75625a087e4e9027b7f311fc8c0805c9
Gerrit-Change-Number: 9194
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] Fix log message when there's too few data dirs for a data dir group

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9194 )

Change subject: Fix log message when there's too few data dirs for a data dir 
group
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I877d335a75625a087e4e9027b7f311fc8c0805c9
Gerrit-Change-Number: 9194
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 22:33:30 +
Gerrit-HasComments: No

[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode

[David Ribeiro Alves (Code Review)]

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

Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
..


Patch Set 10:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto
File src/kudu/common/common.proto:

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@222
PS10, Line 222:  snapshot
nit: snapshot timestamp


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@228
PS10, Line 228: take
nit: choose


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@229
PS10, Line 229: which
nit: such that


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1922
PS10, Line 1922: TestScanYourWrites
We decided we also wanted scan your reads right? is that covered?


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1944
PS10, Line 1944:   // Send the call
I know this is likely copy/paste but remove (adds nothing)


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1953
PS10, Line 1953:   // Make sure that there is no snapshot timestamp sent back.
   :   ASSERT_TRUE(!resp.has_snap_timestamp());
I'm still unsure why we wouldn't the response to include the timestamp. I agree 
that the scan itself (seen as a collection os scans to multiple tablets) can't 
be described by a single timestamp, but having the timestamp in the response 
would allows to have fault-tolerant scans, right?


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1989
PS10, Line 1989:   // Send the call
same


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.h@158
PS10, Line 158: on the read mode,
on the read mode?


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2107
PS10, Line 2107: s.IsNotSupported()
nit add a predict false


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2161
PS10, Line 2161: 0
use kMinTimestamp or something? I understand you can't use kInvalidTimestamp 
(it's big) but don't want to accidentally set time to 0 (been there done that 
:))


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2163
PS10, Line 2163: tmp_snap_timestamp > clean_timestamp ?
   :  tmp_snap_timestamp : clean_timestamp;
std:max ?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Fri, 02 Feb 2018 22:33:39 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2281: Add Primary Key Support For INT128 Columns

[Grant Henke (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: KUDU-2281: Add Primary Key Support For INT128 Columns
..

KUDU-2281: Add Primary Key Support For INT128 Columns

Change-Id: Ic6aadca2c5207f397ef6bdde4462a7497fea8ccd
---
M src/kudu/client/predicate-test.cc
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/common/encoded_key-test.cc
M src/kudu/common/key_encoder.cc
M src/kudu/common/key_encoder.h
M src/kudu/common/key_util-test.cc
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.h
M src/kudu/integration-tests/all_types-itest.cc
10 files changed, 85 insertions(+), 9 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic6aadca2c5207f397ef6bdde4462a7497fea8ccd
Gerrit-Change-Number: 9199
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

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

[Grant Henke (Code Review)]

Grant Henke has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8830 )

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

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

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Reviewed-on: http://gerrit.cloudera.org:8080/8830
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
Reviewed-by: Grant Henke 
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,356 insertions(+), 171 deletions(-)

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

--
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: merged
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 33
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

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

[Grant Henke (Code Review)]

Grant Henke 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 32: Code-Review+2

Thanks everyone for the review. I am going to submit this.

This is just the start of the feature. I will be adding a lot of follow on 
patches. If you have more feedback or review let me know and I can include it 
in a new smaller patch.


--
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: 32
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 22:04:18 +
Gerrit-HasComments: No

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

[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 32: Code-Review+2

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.cc@190
PS28, Line 190:   delete data_;
  :   data_ = new Data(*other.data_);
> All instances of CopyFrom in schema.cc behave this way. The = operator does
SGTM: yep, it's better to keep in uniform in that case.


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

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/common/schema.h@91
PS28, Line 91:  public:
> I was following "convention" of the other structs in this file. Should we d
I tried to locate corresponding section in 
https://google.github.io/styleguide/cppguide.html, but didn't see anything.

It's just a nit and if it's inline with the rest of the stuff, it's perfectly 
fine with me.


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc
File src/kudu/integration-tests/decimal-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@55
PS28, Line 55: eName = "decimal-ta
> Further down I set num_replicas to kNumServers.
Ah, ok -- it seems I missed it.



--
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: 32
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 21:58:12 +
Gerrit-HasComments: Yes

[kudu-CR] Fix log message when there's too few data dirs for a data dir group

[Will Berkeley (Code Review)]

Hello Kudu Jenkins, Andrew Wong,

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

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

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

Change subject: Fix log message when there's too few data dirs for a data dir 
group
..

Fix log message when there's too few data dirs for a data dir group

The address of the dirs full and dirs failed metrics were being
logged instead of the metric value:

I0201 14:45:22.566895  3316 data_dirs.cc:930] Could only allocate 1 dirs of 
requested 3 for tablet 78a4a8db9829474e84b7a5474c9114fd. 1 dirs total, 
0x379c750 dirs full, 0x37599b0 dirs failed

Change-Id: I877d335a75625a087e4e9027b7f311fc8c0805c9
---
M src/kudu/fs/data_dirs.cc
1 file changed, 4 insertions(+), 3 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I877d335a75625a087e4e9027b7f311fc8c0805c9
Gerrit-Change-Number: 9194
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Fix log message when there's too few data dirs for a data dir group

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9194 )

Change subject: Fix log message when there's too few data dirs for a data dir 
group
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9194/1/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:

http://gerrit.cloudera.org:8080/#/c/9194/1/src/kudu/fs/data_dirs.cc@928
PS1, Line 928: $1 dirs full, $2
> $0 and $1
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I877d335a75625a087e4e9027b7f311fc8c0805c9
Gerrit-Change-Number: 9194
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 21:46:10 +
Gerrit-HasComments: Yes

[kudu-CR] Fix log message when there's too few data dirs for a data dir group

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9194 )

Change subject: Fix log message when there's too few data dirs for a data dir 
group
..


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9194/1/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:

http://gerrit.cloudera.org:8080/#/c/9194/1/src/kudu/fs/data_dirs.cc@928
PS1, Line 928: $1 dirs full, $2
$0 and $1



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I877d335a75625a087e4e9027b7f311fc8c0805c9
Gerrit-Change-Number: 9194
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 02 Feb 2018 21:35:07 +
Gerrit-HasComments: Yes

[kudu-CR] maintenance manager: log the reason for scheduling each operation

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9172 )

Change subject: maintenance_manager: log the reason for scheduling each 
operation
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9172/1/src/kudu/util/maintenance_manager.cc
File src/kudu/util/maintenance_manager.cc:

http://gerrit.cloudera.org:8080/#/c/9172/1/src/kudu/util/maintenance_manager.cc@444
PS1, Line 444: best performance improvement
> It's not just this log that worries me but it's the one that worries me the
+1 to making the messages concise, but we should include the memory % in the 
memory pressure messages, since it's actually hard to know from the logs w/o 
seeing rejections.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4dcdb863a7a0b0fc2a72757801d5c057fa725c34
Gerrit-Change-Number: 9172
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 21:25:22 +
Gerrit-HasComments: Yes

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

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

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

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

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

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

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

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,356 insertions(+), 171 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/32
--
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: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 32
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] jsonwriter: do some internal buffering in front of ostringstream

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9181 )

Change subject: jsonwriter: do some internal buffering in front of ostringstream
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib45d58187cf80ecfe2f5a25e40d7042e4a27619d
Gerrit-Change-Number: 9181
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 21:17:02 +
Gerrit-HasComments: No

[kudu-CR] jsonwriter: small optimization for repeated PB fields

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9184 )

Change subject: jsonwriter: small optimization for repeated PB fields
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iceee0096a3af9300fca3dd18508a12e140e99b31
Gerrit-Change-Number: 9184
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 21:12:31 +
Gerrit-HasComments: No

[kudu-CR] metrics: fast-path PB/JSON dump for empty histograms

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9179 )

Change subject: metrics: fast-path PB/JSON dump for empty histograms
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0732e8299e2f27c7c6021ec70740984ab46a69c0
Gerrit-Change-Number: 9179
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 21:01:08 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2281: Add Primary Key Support For INT128 Columns

[Grant Henke (Code Review)]

Grant Henke has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9199


Change subject: KUDU-2281: Add Primary Key Support For INT128 Columns
..

KUDU-2281: Add Primary Key Support For INT128 Columns

Change-Id: Ic6aadca2c5207f397ef6bdde4462a7497fea8ccd
---
M src/kudu/client/predicate-test.cc
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/common/encoded_key-test.cc
M src/kudu/common/key_encoder.cc
M src/kudu/common/key_encoder.h
M src/kudu/common/key_util-test.cc
M src/kudu/common/key_util.cc
M src/kudu/common/partial_row.h
M src/kudu/integration-tests/all_types-itest.cc
10 files changed, 84 insertions(+), 9 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic6aadca2c5207f397ef6bdde4462a7497fea8ccd
Gerrit-Change-Number: 9199
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke 

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

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

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

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

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

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

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

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,355 insertions(+), 171 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/30
--
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: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 30
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2279 (part 5): don't include entity attributes in log

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9180 )

Change subject: KUDU-2279 (part 5): don't include entity attributes in log
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic995f6a3b8430b73a041de8e5bcbc53a5d8e7f1b
Gerrit-Change-Number: 9180
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:47:10 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2279 (part 3): metrics: don't emit untouched metrics to log

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9177 )

Change subject: KUDU-2279 (part 3): metrics: don't emit untouched metrics to log
..


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9177/1/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:

http://gerrit.cloudera.org:8080/#/c/9177/1/src/kudu/util/metrics.h@434
PS1, Line 434: f 0. Note that some metrics with the value 0
aren't there counters we increment by negative amounts



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I92d2c4640e54c91791fab9c420215bafa3fe8f20
Gerrit-Change-Number: 9177
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:45:41 +
Gerrit-HasComments: Yes

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

[Grant Henke (Code Review)]

Grant Henke 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 28:

(8 comments)

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

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.h@244
PS28, Line 244:   /// This constructor is private because clients should use 
the Builder API.
  :   ///
  :   /// @param [in] name
  :   ///   The name of the column.
  :   /// @param [in] type
  :   ///   The type of the column.
  :   /// @param [in] is_nullable
  :   ///   Whether the column is nullable.
  :   /// @param [in] default_value
  :   ///   Default value for the column.
  :   /// @param [in] attributes
  :   ///   Column storage attributes.
> nit: usually we don't document private methods since they are not accessibl
Done


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

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.cc@190
PS28, Line 190:   delete data_;
  :   data_ = new Data(*other.data_);
> What if other == *this?
All instances of CopyFrom in schema.cc behave this way. The = operator does 
have the check before using it.


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

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/common/schema.h@91
PS28, Line 91:  public:
> nit: it's public by default in struct, no need to specify explicitly
I was following "convention" of the other structs in this file. Should we 
define this as style rule?


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc
File src/kudu/integration-tests/decimal-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@52
PS28, Line 52: const int kNumServers = 3;
> nit: it's 3 tablet server by default, you could drop this.
Further down I set num_replicas to kNumServers.


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@55
PS28, Line 55: {}, {}, kNumServers
> nit: I think it's possible to drop all this and use the parameters 'by defa
Further down I set num_replicas to kNumServers.


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@98
PS28, Line 98:   client_->OpenTable(kTableName, );
> nit: ASSERT_OK ?
Done


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@130
PS28, Line 130:   ASSERT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT));
> nit: it's possible to drop this since SetFaultTolerant() set the read mode
Done


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@172
PS28, Line 172: }
> nit: would it make sense to add an 'erroneous case' trying to read decimal
Done



--
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: 28
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:35:36 +
Gerrit-HasComments: Yes

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

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

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

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

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

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

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

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,355 insertions(+), 171 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/29
--
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: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 29
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] rpc: micro-optimize delayed task handling

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9048 )

Change subject: rpc: micro-optimize delayed task handling
..


Patch Set 3: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Gerrit-Change-Number: 9048
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:30:12 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9176 )

Change subject: KUDU-2279 (part 2): metrics: only emit changed metrics in 
metrics log
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h@595
PS2, Line 595: auto cur = current_epoch();
 : if (base::subtle::NoBarrier_Load(_epoch_) < cur) {
 :   base::subtle::NoBarrier_Store(_epoch_, cur);
 : }
> I think the following interleaving could happen here:
Sorry, this interleaving isn't right.

First, suppose m_epoch_ starts at -1 (or increment everything by 1 and start it 
at 0).

Second, T1 has to do its test, before T2 runs at all, then do the set after T2 
finishes.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
Gerrit-Change-Number: 9176
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:21:19 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9176 )

Change subject: KUDU-2279 (part 2): metrics: only emit changed metrics in 
metrics log
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.cc@357
PS2, Line 357: Metric::IncrementEpoch();
Also, if we want to make sure hitting /metrics doesn't change the epoch, we 
should add a quick test for it.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
Gerrit-Change-Number: 9176
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:17:13 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9176 )

Change subject: KUDU-2279 (part 2): metrics: only emit changed metrics in 
metrics log
..


Patch Set 2:

(10 comments)

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

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/server/server_base.cc@623
PS2, Line 623: buf << "metrics " << GetCurrentTimeMicros() << " ";
The "metrics" string here is redundant for the metrics log. Plus, I think we 
should have each line of the metrics log be valid json, so there's one less 
step for a user to process it line-by-line. Can the time go into the metrics 
json?


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/server/server_base.cc@630
PS2, Line 630: opts.only_modified_since_epoch = prev_log_epoch + 1;
 : int64_t this_log_epoch = Metric::current_epoch();
 : Metric::IncrementEpoch();
If `Metric::current_epoch()` is initially 1, we actually jump epochs in 
`opts.only_modified_since_epoch` from 0 to 2. Besides, I think this code could 
be simpler and just set opts.only_modified_since_epoch to 
`Metric::current_epoch()` just before incrementing the current epoch.


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

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics-test.cc@330
PS2, Line 330: bytes_seen = METRIC_reqs_pending
Mismatched names? Also, I'm weakly in favor of having canned metric names 
whenever the metric is fake or its identity is unimportant, like when testing 
metric subsystem internals. E.g. "test_counter" for counters.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics-test.cc@332
PS2, Line 332: epoch_before_modify
This is the epoch of/containing the modification, not the epoch before.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics-test.cc@347
PS2, Line 347: epoch_before_modify
This should be `new_epoch`.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h@567
PS2, Line 567: it's
Remove.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h@570
PS2, Line 570: base::subtle::NoBarrier_Load(_epoch_)
Why use these atomics and not C++11 atomics? (Actual question, not 
review-demand-phrased-as-question :))


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.h@595
PS2, Line 595: auto cur = current_epoch();
 : if (base::subtle::NoBarrier_Load(_epoch_) < cur) {
 :   base::subtle::NoBarrier_Store(_epoch_, cur);
 : }
I think the following interleaving could happen here:

current epoch = 0 initially
Metrics log threadT1  T2
  incr metric
  cur = 0
log for >= epoch 0
epoch++
incr metric
cur = 1
TAS m_epoch_ = 1
  TAS m_epoch_ = 0
log for >= epoch 1
epoch++

so now we might fail to log the metric's new values in epoch 1, and only see 
values from epoch >= 2 if the metric changes again.

Are you accepting this risk to keep the metric lock-free? AFAIK to fix it 
requires a bona fide lock and not just atomic ops.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.cc
File src/kudu/util/metrics.cc:

http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.cc@357
PS2, Line 357: Metric::IncrementEpoch();
With this here we increment the epoch every time someone hits /metrics and we 
double increment the current epoch when we log metrics, both of which can cause 
the metrics logging to miss metrics history. I think we just want to increment 
the epoch in the metrics logging thread.

Also, for another patch, might be nice to expose an epoch parameter in 
/metrics, to enable incremental gathering by a collector that can remember its 
epoch.


http://gerrit.cloudera.org:8080/#/c/9176/2/src/kudu/util/metrics.cc@499
PS2, Line 499: 1
Can we start at 0?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
Gerrit-Change-Number: 9176
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:16:10 +
Gerrit-HasComments: Yes

[kudu-CR] [tests] Add more test coverage for INT128 columns

[Grant Henke (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: [tests] Add more test coverage for INT128 columns
..

[tests] Add more test coverage for INT128 columns

Improves test coverage of reading and writing INT128
columns.

Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/partial_row.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
6 files changed, 49 insertions(+), 6 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] [tests] Add more test coverage for INT128 columns

[Grant Henke (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: [tests] Add more test coverage for INT128 columns
..

[tests] Add more test coverage for INT128 columns

Improves test coverage of reading and writing INT128
columns.

Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/partial_row.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
6 files changed, 48 insertions(+), 6 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

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

[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 28:

(8 comments)

LGTM, just a few nits.

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

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.h@244
PS28, Line 244:   /// This constructor is private because clients should use 
the Builder API.
  :   ///
  :   /// @param [in] name
  :   ///   The name of the column.
  :   /// @param [in] type
  :   ///   The type of the column.
  :   /// @param [in] is_nullable
  :   ///   Whether the column is nullable.
  :   /// @param [in] default_value
  :   ///   Default value for the column.
  :   /// @param [in] attributes
  :   ///   Column storage attributes.
nit: usually we don't document private methods since they are not accessible 
via client API.


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

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/schema.cc@190
PS28, Line 190:   delete data_;
  :   data_ = new Data(*other.data_);
What if other == *this?


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

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/common/schema.h@91
PS28, Line 91:  public:
nit: it's public by default in struct, no need to specify explicitly


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc
File src/kudu/integration-tests/decimal-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@52
PS28, Line 52: const int kNumServers = 3;
nit: it's 3 tablet server by default, you could drop this.


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@55
PS28, Line 55: {}, {}, kNumServers
nit: I think it's possible to drop all this and use the parameters 'by default'


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@98
PS28, Line 98:   client_->OpenTable(kTableName, );
nit: ASSERT_OK ?


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@130
PS28, Line 130:   ASSERT_OK(scanner.SetReadMode(KuduScanner::READ_AT_SNAPSHOT));
nit: it's possible to drop this since SetFaultTolerant() set the read mode to 
READ_AT_SNAPSHOT 'automagically'.


http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/integration-tests/decimal-itest.cc@172
PS28, Line 172: }
nit: would it make sense to add an 'erroneous case' trying to read decimal as, 
say, float/double/string/ etc.?



--
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: 28
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 19:25:00 +
Gerrit-HasComments: Yes

[kudu-CR] Fix log message when there's too few data dirs for a data dir group

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9194 )

Change subject: Fix log message when there's too few data dirs for a data dir 
group
..


Patch Set 1: Code-Review+2

Thanks for catching that!


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I877d335a75625a087e4e9027b7f311fc8c0805c9
Gerrit-Change-Number: 9194
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 02 Feb 2018 19:05:05 +
Gerrit-HasComments: No

[kudu-CR] Fix log message when there's too few data dirs for a data dir group

[Will Berkeley (Code Review)]

Will Berkeley has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9194


Change subject: Fix log message when there's too few data dirs for a data dir 
group
..

Fix log message when there's too few data dirs for a data dir group

The address of the dirs full and dirs failed metrics were being
logged instead of the metric value:

I0201 14:45:22.566895  3316 data_dirs.cc:930] Could only allocate 1 dirs of 
requested 3 for tablet 78a4a8db9829474e84b7a5474c9114fd. 1 dirs total, 
0x379c750 dirs full, 0x37599b0 dirs failed

Change-Id: I877d335a75625a087e4e9027b7f311fc8c0805c9
---
M src/kudu/fs/data_dirs.cc
1 file changed, 4 insertions(+), 3 deletions(-)



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

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

[kudu-CR] Don't rely on pending config OpId index for peer promotion

[Alexey Serbin (Code Review)]

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

Change subject: Don't rely on pending config OpId index for peer promotion
..


Patch Set 3: Code-Review+1

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9161/3/src/kudu/consensus/raft_consensus.cc@864
PS3, Line 864: req.set_cas_config_opid_index(current_committed_config_index);
> We want to retain it so when the master says to add a new node we ignore th
Ah, than makes sense.  I thought there is a separate code path for that.  
Having just one is better, of course.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a033ce16f626458efdb59b1b4dd8450e8591761
Gerrit-Change-Number: 9161
Gerrit-PatchSet: 3
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 18:41:57 +
Gerrit-HasComments: Yes

[kudu-CR] [tests] Add more test coverage for INT128 columns

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9193 )

Change subject: [tests] Add more test coverage for INT128 columns
..


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/cfile/cfile-test.cc@553
PS2, Line 553:
> no need for this space between closing brackets anymore; this was fixed in
Done


http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/tablet/all_types-scan-correctness-test.cc
File src/kudu/tablet/all_types-scan-correctness-test.cc:

http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/tablet/all_types-scan-correctness-test.cc@585
PS2, Line 585:  // TODO: Uncomment when adding 128 bit 
support to RLE
> Probably worth having a tracking ticket for this since I imagine we won't d
Agreed. Created KUDU-2284 and updated all places where this was used.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 02 Feb 2018 18:41:10 +
Gerrit-HasComments: Yes

[kudu-CR] [tests] Add more test coverage for INT128 columns

[Grant Henke (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: [tests] Add more test coverage for INT128 columns
..

[tests] Add more test coverage for INT128 columns

Improves test coverage of reading and writing INT128
columns.

Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/common/partial_row.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
5 files changed, 46 insertions(+), 6 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] [tests] Add more test coverage for INT128 columns

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9193 )

Change subject: [tests] Add more test coverage for INT128 columns
..


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/cfile/cfile-test.cc@553
PS2, Line 553:
no need for this space between closing brackets anymore; this was fixed in 
C++11.


http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/tablet/all_types-scan-correctness-test.cc
File src/kudu/tablet/all_types-scan-correctness-test.cc:

http://gerrit.cloudera.org:8080/#/c/9193/2/src/kudu/tablet/all_types-scan-correctness-test.cc@585
PS2, Line 585:  // TODO: Uncomment when adding 128 bit 
support to RLE
> warning: missing username/bug in TODO [google-readability-todo]
Probably worth having a tracking ticket for this since I imagine we won't do it 
right away.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 02 Feb 2018 18:28:14 +
Gerrit-HasComments: Yes

[kudu-CR] [tests] Add more test coverage for INT128 columns

[Grant Henke (Code Review)]

Hello Tidy Bot, Kudu Jenkins,

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

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

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

Change subject: [tests] Add more test coverage for INT128 columns
..

[tests] Add more test coverage for INT128 columns

Improves test coverage of reading and writing INT128
columns.

Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/common/partial_row.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
5 files changed, 41 insertions(+), 1 deletion(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] [docs] Added recommendation to compress PK for backfill inserts

[Alex Rodoni (Code Review)]

Alex Rodoni has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9185


Change subject: [docs] Added recommendation to compress PK for backfill inserts
..

[docs] Added recommendation to compress PK for backfill inserts

Change-Id: I698d954265b4171e4d1bb7e01e286d0d489f1ec7
---
M docs/schema_design.adoc
1 file changed, 64 insertions(+), 18 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I698d954265b4171e4d1bb7e01e286d0d489f1ec7
Gerrit-Change-Number: 9185
Gerrit-PatchSet: 2
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Dan Burkert 

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

[Dan Burkert (Code Review)]

Dan Burkert 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 28: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc@978
PS28, Line 978: TEST_F(PredicateTest, TestDecimalPredicates) {
> The follow up patch to support int128 keys is actually required to support
OK sounds good!  I'm fine with landing this as-is as long as there's a path 
forwards.



--
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: 28
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 18:08:15 +
Gerrit-HasComments: Yes

[kudu-CR] [tests] Add more test coverage for INT128 columns

[Grant Henke (Code Review)]

Grant Henke has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9193


Change subject: [tests] Add more test coverage for INT128 columns
..

[tests] Add more test coverage for INT128 columns

Improves test coverage of reading and writing INT128
columns.

Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/encoding-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/common/partial_row.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
5 files changed, 40 insertions(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3350634e55aad4316036c80d03e95e0acdf64295
Gerrit-Change-Number: 9193
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke 

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

[Grant Henke (Code Review)]

Grant Henke 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 28:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc@978
PS28, Line 978: TEST_F(PredicateTest, TestDecimalPredicates) {
> Sorry to keep harping on this, but I still feel we may be missing some impo
The follow up patch to support int128 keys is actually required to support this 
tests using decimal128 because the predicates eventually call 
key_util::IncrementCell. I have that patch almost ready to push where I also 
update this test to use DECIMAL128. I will publish it shortly.

I also have a separate patch increasing generic INT128 test coverage that I 
will push today too.



--
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: 28
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 17:59:55 +
Gerrit-HasComments: Yes

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

[Dan Burkert (Code Review)]

Dan Burkert 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 28:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc
File src/kudu/client/predicate-test.cc:

http://gerrit.cloudera.org:8080/#/c/8830/28/src/kudu/client/predicate-test.cc@978
PS28, Line 978: TEST_F(PredicateTest, TestDecimalPredicates) {
Sorry to keep harping on this, but I still feel we may be missing some 
important predicate coverage.  IIUC this is only covering Decimal32, right?  
I'd like to at least see Decimal128 coverage as well, since that's a completely 
new physical type.

In the follow up patch that does decimal primary keys we should make sure we 
have predicate coverage over decimal PKs as well, since the predicate 
optimization over PKs is extra special.



--
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: 28
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Feb 2018 17:42:51 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8804 )

Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
..


Patch Set 10:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto
File src/kudu/common/common.proto:

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@231
PS10, Line 231: stale
The 'stale' terminology hasn't been introduced here, so I think it would be 
best to avoid it.  Perhaps 'two READ_YOUR_WRITES scans, ...'


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@233
PS10, Line 233: inconsistent
I think it would be better to say 'different results' here in order to avoid 
confusion over the formal consistency guarantees.


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@237
PS10, Line 237:   READ_YOUR_WRITES = 3;
It may be good to add docs about whether a snapshot timestamp and propagated 
timestamp are returned, and what the client should do with them.

I'm surprised to see in the latest revision that READ_YOUR_WRITES doesn't 
return a snapshot timestamp, why is that?  And is the returned propagated 
timestamp set to the snapshot timestamp of the scan?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Fri, 02 Feb 2018 17:17:09 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2279 (part 4): enable metrics log by default

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9178 )

Change subject: KUDU-2279 (part 4): enable metrics log by default
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaeb4c546c2800d8f4288a3626b9e998cd7e35e5c
Gerrit-Change-Number: 9178
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 16:38:58 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2279 (part 1). rolling log: respect logfile retention flag

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9175 )

Change subject: KUDU-2279 (part 1). rolling_log: respect logfile retention flag
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If3cc9b085c9827fd544b0bf7d7ae868e3692296b
Gerrit-Change-Number: 9175
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Feb 2018 16:22:33 +
Gerrit-HasComments: No