[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8338 )

Change subject: log block manager: return early if read-only
..


Patch Set 5: Verified+1

Agreed. Overriding Jenkins.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 21 Oct 2017 01:09:58 +
Gerrit-HasComments: No

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Adar Dembo (Code Review) 
Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8338 )

Change subject: log block manager: return early if read-only
..

log block manager: return early if read-only

A container may be used for multiple block transactions, and when any
transaction is committed, the blocks in that transaction are closed.  If
blocks in a container fail to close, the container is marked read-only,
as this may cause on-disk inconsistencies.

If there are multiple transactions affecting the same container, a
container may be marked read-only by one transaction while still being
written to by another, leading to a DCHECK failure.

This patch changes this by storing a Status to indicate read-only
instead of an AtomicBool. Functions that previously DCHECKed the
container was not read-only now return early with the stored error and
appropriate error-handling. Additionally, a couple of checks for
read-only have been moved out of the blocks and into to the containers
themselves.

A test is added that shares a single container among multiple
transactions, injects failure in one, and ensures the failure is visible
across the other transactions.

Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Reviewed-on: http://gerrit.cloudera.org:8080/8338
Reviewed-by: Adar Dembo 
Reviewed-by: Hao Hao 
Tested-by: Adar Dembo 
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 123 insertions(+), 44 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved; Verified
  Hao Hao: Looks good to me, but someone else must approve

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Adar Dembo (Code Review) 
Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/8338 )

Change subject: log block manager: return early if read-only
..


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8338
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Andrew Wong (Code Review) 
Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8338 )

Change subject: log block manager: return early if read-only
..


Patch Set 5:

> Patch Set 5:
>
> (4 comments)

Failure seems unrelated and I think is a flake due to a timeout + slow thread 
creation (although haven't seen it on the dash).


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 21 Oct 2017 00:46:36 +
Gerrit-HasComments: No

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Hao Hao (Code Review) 
Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8338 )

Change subject: log block manager: return early if read-only
..


Patch Set 5: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 21 Oct 2017 00:36:09 +
Gerrit-HasComments: No

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8338 )

Change subject: log block manager: return early if read-only
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 21 Oct 2017 00:32:27 +
Gerrit-HasComments: No

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Andrew Wong (Code Review) 
Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8338 )

Change subject: log block manager: return early if read-only
..


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8338/4/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:

http://gerrit.cloudera.org:8080/#/c/8338/4/src/kudu/fs/log_block_manager.h@112
PS4, Line 112: transac
> Change it to transaction to be more specific?
Done


http://gerrit.cloudera.org:8080/#/c/8338/4/src/kudu/fs/log_block_manager.h@118
PS4, Line 118: above
> Nit: since it's not in the immediate paragraph "above" maybe change to "afo
changed to the "above commit", which is in the immediate above paragraph


http://gerrit.cloudera.org:8080/#/c/8338/4/src/kudu/fs/log_block_manager.h@122
PS4, Line 122:  pot
> And also mention could corrupt the underlying container?
Done


http://gerrit.cloudera.org:8080/#/c/8338/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8338/4/src/kudu/fs/log_block_manager.cc@1842
PS4, Line 1842: HANDLE_DISK_FAILURE(lb->container()->read_only_status(),
> Am I correct in my understanding that this will fail the data directory syn
Yes that will be the case. With the proper locking in error handling (not yet 
pushed), these wrapped calls _should_ ensure that error handling has completed 
upon return.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 21 Oct 2017 00:12:06 +
Gerrit-HasComments: Yes

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Andrew Wong (Code Review) 
Hello Dan Burkert, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: log block manager: return early if read-only
..

log block manager: return early if read-only

A container may be used for multiple block transactions, and when any
transaction is committed, the blocks in that transaction are closed.  If
blocks in a container fail to close, the container is marked read-only,
as this may cause on-disk inconsistencies.

If there are multiple transactions affecting the same container, a
container may be marked read-only by one transaction while still being
written to by another, leading to a DCHECK failure.

This patch changes this by storing a Status to indicate read-only
instead of an AtomicBool. Functions that previously DCHECKed the
container was not read-only now return early with the stored error and
appropriate error-handling. Additionally, a couple of checks for
read-only have been moved out of the blocks and into to the containers
themselves.

A test is added that shares a single container among multiple
transactions, injects failure in one, and ensures the failure is visible
across the other transactions.

Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 123 insertions(+), 44 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8338 )

Change subject: log block manager: return early if read-only
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8338/4/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:

http://gerrit.cloudera.org:8080/#/c/8338/4/src/kudu/fs/log_block_manager.h@118
PS4, Line 118: above
Nit: since it's not in the immediate paragraph "above" maybe change to 
"aforementioned"?


http://gerrit.cloudera.org:8080/#/c/8338/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8338/4/src/kudu/fs/log_block_manager.cc@1842
PS4, Line 1842: HANDLE_DISK_FAILURE(lb->container()->read_only_status(),
Am I correct in my understanding that this will fail the data directory 
synchronously, so that the code on L1846-L1855 returns out of the function?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 20 Oct 2017 23:54:04 +
Gerrit-HasComments: Yes

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Hao Hao (Code Review) 
Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8338 )

Change subject: log block manager: return early if read-only
..


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8338/3/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:

http://gerrit.cloudera.org:8080/#/c/8338/3/src/kudu/fs/log_block_manager-test.cc@888
PS3, Line 888: auto&
Can you use const auto&?


http://gerrit.cloudera.org:8080/#/c/8338/4/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:

http://gerrit.cloudera.org:8080/#/c/8338/4/src/kudu/fs/log_block_manager.h@112
PS4, Line 112: writers
Change it to transaction to be more specific?


http://gerrit.cloudera.org:8080/#/c/8338/4/src/kudu/fs/log_block_manager.h@122
PS4, Line 122: fail
And also mention could corrupt the underlying container?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 20 Oct 2017 23:47:47 +
Gerrit-HasComments: Yes

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Andrew Wong (Code Review) 
Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8338 )

Change subject: log block manager: return early if read-only
..


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8338/3/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:

http://gerrit.cloudera.org:8080/#/c/8338/3/src/kudu/fs/log_block_manager.h@104
PS3, Line 104: block is
> This was actually more correct as 'block', I think, because 'available' her
Ah gotcha.


http://gerrit.cloudera.org:8080/#/c/8338/3/src/kudu/fs/log_block_manager.h@108
PS3, Line 108: // Writes to containers are batched together through the use of 
block
> I'd reword this to downplay "container availabiliity" and instead to descri
Done


http://gerrit.cloudera.org:8080/#/c/8338/3/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8338/3/src/kudu/fs/log_block_manager.cc@1842
PS3, Line 1842: HANDLE_DISK_FAILURE(lb->container()->read_only_status(),
  : 
error_manager_->RunErrorNotificationCb(lb->container()->data_dir()));
  :
  : /
> Do we want to use the read_only_status_ here?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 20 Oct 2017 22:31:28 +
Gerrit-HasComments: Yes

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Andrew Wong (Code Review) 
Hello Dan Burkert, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: log block manager: return early if read-only
..

log block manager: return early if read-only

A container may be used for multiple block transactions, and when any
transaction is committed, the blocks in that transaction are closed.  If
blocks in a container fail to close, the container is marked read-only,
as this may cause on-disk inconsistencies.

If there are multiple transactions affecting the same container, a
container may be marked read-only by one transaction while still being
written to by another, leading to a DCHECK failure.

This patch changes this by storing a Status to indicate read-only
instead of an AtomicBool. Functions that previously DCHECKed the
container was not read-only now return early with the stored error and
appropriate error-handling. Additionally, a couple of checks for
read-only have been moved out of the blocks and into to the containers
themselves.

A test is added that shares a single container among multiple
transactions, injects failure in one, and ensures the failure is visible
across the other transactions.

Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 122 insertions(+), 44 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Hao Hao (Code Review) 
Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8338 )

Change subject: log block manager: return early if read-only
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8338/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8338/2/src/kudu/fs/log_block_manager.cc@1062
PS2, Line 1062: ner's next bl
> On second thought, maybe it's worth more consideration. Should the below bo
Hmm, right. I forgot/missed that MakeContainerAvailable checks if it's 
read-only. Then it looks good to me.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 20 Oct 2017 21:52:30 +
Gerrit-HasComments: Yes

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8338 )

Change subject: log block manager: return early if read-only
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8338/3/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:

http://gerrit.cloudera.org:8080/#/c/8338/3/src/kudu/fs/log_block_manager.h@104
PS3, Line 104: container
This was actually more correct as 'block', I think, because 'available' here 
isn't referring to making a container available for another writer, but making 
the block available to readers.


http://gerrit.cloudera.org:8080/#/c/8338/3/src/kudu/fs/log_block_manager.h@108
PS3, Line 108: // The current container-availability semantics enforce that a 
single container
I'd reword this to downplay "container availabiliity" and instead to describe 
this in terms of the public API, drawing upon block finalization as the 
operation that allows a new entity to start writing to the container.


http://gerrit.cloudera.org:8080/#/c/8338/3/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8338/3/src/kudu/fs/log_block_manager.cc@1842
PS3, Line 1842: if (lb->container()->read_only()) {
  :   return Status::IllegalState(Substitute("container $0 is 
read-only",
  :  
lb->container()->ToString()));
  : }
Do we want to use the read_only_status_ here?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 20 Oct 2017 21:47:09 +
Gerrit-HasComments: Yes

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Andrew Wong (Code Review) 
Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8338 )

Change subject: log block manager: return early if read-only
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8338/2/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

http://gerrit.cloudera.org:8080/#/c/8338/2/src/kudu/fs/log_block_manager.cc@896
PS2, Line 896:   RETURN_NOT_OK_HANDLE_ERROR(read_only_status());
> Maybe not necessary, because the operations in sync_blocks() will do the sa
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 20 Oct 2017 21:40:13 +
Gerrit-HasComments: Yes

[kudu-CR] log block manager: return early if read-only

2017-10-20 Thread Andrew Wong (Code Review) 
Hello Dan Burkert, Kudu Jenkins, Adar Dembo, Hao Hao,

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

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

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

Change subject: log block manager: return early if read-only
..

log block manager: return early if read-only

A container may be used for multiple block transactions, and when any
transaction is committed, the blocks in that transaction are closed.  If
blocks in a container fail to close, the container is marked read-only,
as this may cause on-disk inconsistencies.

If there are multiple transactions affecting the same container, a
container may be marked read-only by one transaction while still being
written to by another, leading to a DCHECK failure.

This patch changes this by storing a Status to indicate read-only
instead of an AtomicBool. Functions that previously DCHECKed the
container was not read-only now return early with the stored error and
appropriate error-handling. Additionally, a couple of checks for
read-only have been moved out of the blocks and into to the containers
themselves.

A test is added that shares a single container among multiple
transactions, injects failure in one, and ensures the failure is visible
across the other transactions.

Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
---
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
3 files changed, 112 insertions(+), 38 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06412a36ed62fe1e3a2307a4a2e7a0a7e3d65702
Gerrit-Change-Number: 8338
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins