[kudu-CR] Don't use InMemoryEnv in deltafile-test

2016-05-27 Thread Mike Percy (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: Don't use InMemoryEnv in deltafile-test
..

Don't use InMemoryEnv in deltafile-test

Get out the vote: #NeverMemEnv.

This is causing problems due to Status::NotSupported for StatVfs() in an
upcoming patch.

Change-Id: I380249e6a72a93e1fde86a551c9d4d32d35904da
---
M src/kudu/tablet/deltafile-test.cc
1 file changed, 6 insertions(+), 8 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I380249e6a72a93e1fde86a551c9d4d32d35904da
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Allow for reserving disk space for non-Kudu processes

2016-05-27 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: Allow for reserving disk space for non-Kudu processes
..


Patch Set 4:

(4 comments)

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

Line 13: reservation limit then a crash will result.
> The problem is that if this guy is a leader then he can maintain his leader
We talked about this on Slack. I commented on this before looking at the actual 
code, and in this patch whenever you check for reserved space you do return 
errors rather than inducing crashes. Perhaps you can reword the description a 
bit so it's more clear that you're referring to the system as a whole rather 
than your new code?

As for your point about leadership and not making progress, I agree, but that's 
more relevant to the previous patch (where you've added LOG(FATAL) in the event 
of an IO failure), so we should continue the discussion there.


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

Line 55: // Note: This functionality is not implemented in the file block 
manager, so
   : // this test is disabled on non-Linux platforms.
> What is the benefit of that? I just basically copied what we do for similar
It's more flexible: even as Kudu platform support evolves and changes, we won't 
ever need to revisit this.

If doing it is a huge pain then I'll relent, but I don't think it is. As for 
the other tests, if I was around to review the Mac OS changes when they landed 
(I think it was while I was away on honeymoon) I would have said the exact same 
thing. :)


http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/util/env.h
File src/kudu/util/env.h:

Line 214:   virtual Status StatVfs(const std::string& path, struct statvfs* 
buf) = 0;
> This was done for expediency; Since this is not an external API it's a deci
Your counterargument is not wrong, but it's one we could use when adding 
anything new to Env. Note that we've always taken care to be platform-agnostic 
here (e.g. Walk() doesn't expose fts* details) and I'd rather we didn't change 
that precedent now, since it's hard to reel back in once undone.

If it helps, I've found that these sort of precedents pay dividends down the 
line, and typically don't result in heavy "taxes" on a patch by patch basis.


http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:

Line 43: Number of inodes to reserve on each filesystem for non-Kudu usage"
> This is something Todd suggested we add. I wasn't sure what the default sho
I'd like to see a stronger justification before this is added. Yes, we can add 
a control for it, but why is it useful? As a system Kudu doesn't really hog 
inodes since it tends to cram large amounts of data into each file, so I don't 
see the use case.

Do you remember what Todd's argument was? Maybe he can chime in.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd0451d4dbddc1783019a53302de0263080939c7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] log: Mark allocation finished even if allocation had an error

2016-05-27 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: log: Mark allocation finished even if allocation had an error
..


Patch Set 1:

(1 comment)

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

Line 206:   LOG(FATAL) << "Error syncing log" << s.ToString();
> Why isn't this one DFATAL? The old code was DLOG(FATAL), which I think is e
We talked about this on Slack. I now understand that your goal was to induce a 
crash here (and presumably above) in the event that appending or syncing fails.

I strongly disagree with this. I empathize with the use case (that a leader 
replica that cannot write to its WAL should be killed), but this isn't the 
right layer for that. If we need to induce a crash when a leader can't write to 
its WAL, we should do that somewhere in the consensus module where we've 
checked that the replica is a leader and not a follower.

In general, we're doing a poor job of isolating Raft consensus details to the 
consensus module. They're spilling out all over the codebase and it's making 
Kudu as a project more difficult to maintain. I'm not trying to pick on you for 
this; obviously we're all to blame. But let's think very carefully before we 
introduce more logic that brings a Raft detail into a module where it 
(ostensibly) doesn't belong.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If22bf946a42d0ec32c35164acd9e6e6cef18dcc3
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow for reserving disk space for non-Kudu processes

2016-05-27 Thread Mike Percy (Code Review) 
Mike Percy has posted comments on this change.

Change subject: Allow for reserving disk space for non-Kudu processes
..


Patch Set 4:

(7 comments)

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

Line 13: reservation limit then a crash will result.
> Why did you choose for the behavior to be a crash instead of just returning
The problem is that if this guy is a leader then he can maintain his leadership 
and never make progress. So it doesn't seem to make sense not to crash.


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

Line 55: // Note: This functionality is not implemented in the file block 
manager, so
   : // this test is disabled on non-Linux platforms.
> Can't we compile the code on all platforms but, at runtime, test the kind o
What is the benefit of that? I just basically copied what we do for similar 
tests.


http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/util/env.h
File src/kudu/util/env.h:

Line 212:   // Returns information about a mounted filesystem.
> No unit test in env-test for StatVfs?
It's literally a pass-through for statvfs().


Line 214:   virtual Status StatVfs(const std::string& path, struct statvfs* 
buf) = 0;
> Not a fan of leaking a POSIX-specific struct into the rest of Kudu. AFAICT 
This was done for expediency; Since this is not an external API it's a decision 
that can be redone later if we need to. However I don't think we have plans to 
support non-POSIX systems anyway so it doesn't seem to be a big problem.


http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

Line 997:   return Status::IOError("statvfs: " + path, 
ErrnoToString(errno), errno);
> You should use the IOError() function so that FLAGS_suicide_on_eio will be 
wasn't aware of that, ok


http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:

Line 43: Number of inodes to reserve on each filesystem for non-Kudu usage"
> Why would an operator care to reserve the number of inodes? Do other system
This is something Todd suggested we add. I wasn't sure what the default should 
be so I defaulted it to 0.

I can imagine a case where we'd want to avoid taking up all the inodes, 
resulting in a difficult to administer system (potentially preventing login?)


Line 125:   if (PREDICT_FALSE(FLAGS_disk_reserved_bytes_free_for_testing > -1)) 
{
> If these for_testing flags are set, why bother with the StatVfs() call at a
It keeps the code simple.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd0451d4dbddc1783019a53302de0263080939c7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Allow for reserving disk space for non-Kudu processes

2016-05-27 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: Allow for reserving disk space for non-Kudu processes
..


Patch Set 4:

(16 comments)

I reviewed everything but the LBM changes. I've indicated enough high-level 
issues that I think we should discuss them first, in case they require some 
reimplementation.

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

Line 13: reservation limit then a crash will result.
Why did you choose for the behavior to be a crash instead of just returning an 
error up the stack? Granted, I doubt we can handle these errors in a meaningful 
way during a flush (if we can't write to disk, we'll run out of memory 
eventually), but we certainly can on a WAL write or a compaction.

In general I think "leaf" code should return errors and "non-leaf" code should, 
if necessary, crash. That way when we do crash we have more context: not only 
do we know we ran out of disk space, but we know we were in the middle of e.g. 
a flush.


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

Line 55: // Note: This functionality is not implemented in the file block 
manager, so
   : // this test is disabled on non-Linux platforms.
Can't we compile the code on all platforms but, at runtime, test the kind of 
block manager we're using, and no-op the test if it's not the LBM?


Line 67:   workload.set_num_write_threads(4);
   :   workload.set_timeout_allowed(true);
   :   workload.set_write_timeout_millis(500);
How did you arrive at these non-default values? If they're important, they're 
worth comments.

num_replicas(1) I understand (you've only got one TS).


Line 73:   // Write a few rows to get some rows flushing.
   :   while (workload.rows_inserted() < 10) {
   : SleepFor(MonoDelta::FromMilliseconds(10));
   :   }
Why do we need to do this if we're also looping and waiting for containers 
below?


Line 80:   // Wait until we have 2 active containers on TS1.
Nit: TS1 is the only TS; why refer to it as TS1? Below too.


Line 95:  1L * 1024 * 1024 * 1024)));
1GB seems unnecessarily high. How about 128 MB, or even lower? How long does 
this test take to run on a spinning disk? On an SSD?


Line 126:   ts_flags.push_back("--enable_maintenance_manager=false"); // No 
flushing for this test.
Why is this important?


Line 132:   workload.set_timeout_allowed(true);
:   workload.set_write_timeout_millis(500);
:   workload.set_num_write_threads(8);
:   workload.set_write_batch_size(1024);
As above, please justify the non-default values with comments.


http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/util/env.h
File src/kudu/util/env.h:

Line 212:   // Returns information about a mounted filesystem.
No unit test in env-test for StatVfs?


Line 214:   virtual Status StatVfs(const std::string& path, struct statvfs* 
buf) = 0;
Not a fan of leaking a POSIX-specific struct into the rest of Kudu. AFAICT this 
is a first.

Could you instead adapt what you need from statvfs into a new Kudu struct or 
class?


http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

Line 993:   virtual Status StatVfs(const std::string& path, struct statvfs* 
buf) OVERRIDE {
Need to add ThreadRestrictions::AssertIOAllowed(). Also should add a 
TRACE_EVENT() call.


Line 997:   return Status::IOError("statvfs: " + path, 
ErrnoToString(errno), errno);
You should use the IOError() function so that FLAGS_suicide_on_eio will be 
honored.


http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:

Line 37: DEFINE_int64(disk_reserved_bytes, 0,
   :  "Number of bytes to reserve on each filesystem for 
non-Kudu usage");
I don't know whether this will be sufficient. I'm thinking we may want to 
configure this separately for WALs and for data blocks. The rationale being: 
WALs live on one device and that's typically a fast, non-spinning device (like 
an SSD or NVRAM). Said device is often smaller and may be used for the OS too, 
so you'd want to handle reservations differently for it. I'm guessing HDFS 
doesn't work this way because all it stores are blocks, and so its data 
directories are uniform more often than not.

This means the reservations should be handled separately in the block managers 
and in the WAL code, and then there's no real use for this centralized code.


Line 43: Number of inodes to reserve on each filesystem for non-Kudu usage"
Why would an operator care to reserve the number of inodes? Do other systems do 
this? What's their rationale?


Line 125:   if (PREDICT_FALSE(FLAGS_disk_reserved_bytes_free_for_testing > -1)) 
{
If these for_testing flags are set, why bother with the StatVfs() call at all?

[kudu-CR] Make BuildLog() return Status

2016-05-27 Thread Mike Percy (Code Review) 
Mike Percy has submitted this change and it was merged.

Change subject: Make BuildLog() return Status
..


Make BuildLog() return Status

This allows for writing a reasonable test related to du.reserved

Change-Id: Icfa8ddba74b909f5697ce03b45e97c1733082b07
Reviewed-on: http://gerrit.cloudera.org:8080/3134
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/consensus/log-test-base.h
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/mt-log-test.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
4 files changed, 37 insertions(+), 36 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Icfa8ddba74b909f5697ce03b45e97c1733082b07
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1307 [master] support tables with range partition bounds

2016-05-27 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change.

Change subject: KUDU-1307 [master] support tables with range partition bounds
..


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/2806/11/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:

Line 107:   for (const pair& bound : bounds) {
: encoder.Add(RowOperationsPB::RANGE_LOWER_BOUND, 
std::get<0>(bound));
: encoder.Add(RowOperationsPB::RANGE_UPPER_BOUND, 
std::get<1>(bound));
:   }
Nit: use auto, and use .first/.second instead of get<0>/get<1>


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I04a7ed3e85c993fdbcb313a419f15031103736ce
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1307 [master] support tables with range partition bounds

2016-05-27 Thread Dan Burkert (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1307 [master] support tables with range partition bounds
..

KUDU-1307 [master] support tables with range partition bounds

This commit adds support to the Kudu master for creating tables with range
partition bounds. Bounds are used to create gaps in the range partition that are
not covered by a tablet. A new master feature flag has been added so that when
client support is added for creating tables with partition bounds, old masters
will not silently ignore the bounds. Adding support to the clients is left to a
follow up commit.

Change-Id: I04a7ed3e85c993fdbcb313a419f15031103736ce
---
M java/kudu-client/src/main/java/org/kududb/client/CreateTableOptions.java
M src/kudu/client/client.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/row_operations.cc
M src/kudu/common/wire_protocol.proto
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/master/master_service.h
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet-harness.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tserver/tablet_server-test.cc
19 files changed, 803 insertions(+), 152 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I04a7ed3e85c993fdbcb313a419f15031103736ce
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1307 [master] support tables with range partition bounds

2016-05-27 Thread Dan Burkert (Code Review) 
Dan Burkert has posted comments on this change.

Change subject: KUDU-1307 [master] support tables with range partition bounds
..


Patch Set 10:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/2806/10//COMMIT_MSG
Commit Message:

Line 11: not covered by a tablet. A new feature master feature flag has been 
added so
> "A new feature master feature flag" is a little awkward, is there an extra 
Done


http://gerrit.cloudera.org:8080/#/c/2806/10/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:

Line 241: Arena arena(1024, 1024 * 1024);
> What's this doing here?
Done


Line 250:   vector>* range_partitions) const {
> To Todd's suggestions, perhaps we should DCHECK(range_partitions->empty()),
Done


Line 264:   "Range bound has lower bound equal to or above the upper 
bound",
> So FWIW, I was wrong in telling you to use capital letters for the beginnin
Done


http://gerrit.cloudera.org:8080/#/c/2806/10/src/kudu/common/partition.h
File src/kudu/common/partition.h:

Line 257: 
> Nit: unnecessary new line?
Done


http://gerrit.cloudera.org:8080/#/c/2806/10/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:

Line 43: namespace kudu {
> Is this typical? Don't we prefer to put all the "using" statements in one g
I think we have it both ways, I'll switch this case.


Line 95:const 
vector Can we use pair here instead of tuple? I agree with Todd that for 2-tuples,
Done


Line 144:   KuduPartialRow row = KuduPartialRow();
> Nit: KuduPartialRow row();
Done


http://gerrit.cloudera.org:8080/#/c/2806/10/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:

Line 411: KuduPartialRow a_lower = KuduPartialRow();
> Nit: these would be more terse without the copy constructor:
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I04a7ed3e85c993fdbcb313a419f15031103736ce
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1470 Exceptions on getting a column value should return the column name not the column number

2016-05-27 Thread Ted Malaska (Code Review) 
Ted Malaska has posted comments on this change.

Change subject: KUDU-1470 Exceptions on getting a column value should return 
the column name not the column number
..


Patch Set 2:

Thanks Dan.  I will have a patch by end of day to fix these issues

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie8bb3db1ed7b4e2027814815776f6252b0f749c5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Ted Malaska 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Ted Malaska 
Gerrit-HasComments: No

[kudu-CR](gh-pages) Add support for anchors.js to get permalinks on all headers

2016-05-27 Thread Mike Percy (Code Review) 
Mike Percy has posted comments on this change.

Change subject: Add support for anchors.js to get permalinks on all headers
..


Patch Set 4: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I17bb50f412d8214f91fc5cf8267823d07db03222
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Misty Stanley-Jones 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 
Gerrit-HasComments: No

[kudu-CR] Allow for reserving disk space for non-Kudu processes

2016-05-27 Thread Mike Percy (Code Review) 
Hello Kudu Jenkins,

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

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

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

Change subject: Allow for reserving disk space for non-Kudu processes
..

Allow for reserving disk space for non-Kudu processes

Adds gflags to reserve disk space such that Kudu will not use more than
specified. Hadoop calls this functionality "du.reserved".

If a WAL preallocation is attempted while the log disk is past its
reservation limit then a crash will result.

The log block manager will use non-full disks if possible until all of
the disks are full. If a flush or compaction is attempted when all disks
are beyond their configured capacity then the process will crash.

This initial implementation provides a "best effort" approach. Disk
space checks are only done at preallocation time, and if writes continue
beyond the preallocated point (for both a WAL segment and a data block)
those writes will not be prevented. This makes it easier to provide a
"friendly" option where the block manager will divert new writes to
non-full disks, avoiding a hard crash when only one disk is past its
reservation limit.

In the future, we may want to add "hard" and "soft" limits, such that
going beyond the soft limit will do what we do today, and going beyond
the hard limit (say, by writing a very large data block past its
preallocation point) will result in a crash.

This patch includes:

* Some unit tests.
* End-to-end test for compaction falling back to non-full disks due to
  disk space backpressure and finally crashing when there is no space
  left in any data dir.
* End-to-end test for writes failing due to WAL disk space backpressure,
  causing a crash.

Change-Id: Ifd0451d4dbddc1783019a53302de0263080939c7
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log.cc
M src/kudu/consensus/log_util.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
A src/kudu/util/env_util-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/memenv/memenv.cc
M src/kudu/util/scoped_cleanup.h
16 files changed, 595 insertions(+), 56 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifd0451d4dbddc1783019a53302de0263080939c7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon