[kudu-CR] kudu-tool-test: adjust minicluster functions slightly

[Adar Dembo (Code Review)]

Hello Andrew Wong, Todd Lipcon,

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

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

to review the following change.


Change subject: kudu-tool-test: adjust minicluster functions slightly
..

kudu-tool-test: adjust minicluster functions slightly

I didn't like that cluster_opts_ was a member of the test fixture. This
patch changes the use of cluster options to be more conventional.

Change-Id: I0f7e613432e165d449e7901be03880c55e8770ab
---
M src/kudu/tools/kudu-tool-test.cc
1 file changed, 20 insertions(+), 26 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0f7e613432e165d449e7901be03880c55e8770ab
Gerrit-Change-Number: 8351
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tool: new actions for adding and removing data directories

[Adar Dembo (Code Review)]

Hello Andrew Wong, Todd Lipcon,

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

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

to review the following change.


Change subject: tool: new actions for adding and removing data directories
..

tool: new actions for adding and removing data directories

To add a data directory, a new FS root is created and within it, a new data
directory. Then, the set of all_uuids is updated to include the uuid of the
new data directory, and all of the on-disk instance files are updated with
the new value of all_uuids.

To remove a data directory, some sanity checks are performed, all_uuids is
updated to exclude the uuid of the data directory being removed, and all of
the on-disk instance files are updated with the new value of all_uuids.
Needless to say, removing a data directory will cause any tablet with data
in it to fail at the next startup.

Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
---
M src/kudu/fs/block_manager_util.h
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
7 files changed, 732 insertions(+), 105 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6ddbdd6cc6231996e7802a622a8b4691527a0643
Gerrit-Change-Number: 8352
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Todd Lipcon 

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

[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

[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

[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

[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

[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] KUDU-2191 (2/n): Hive Metastore client

[Alexey Serbin (Code Review)]

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

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 29:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.cc@26
PS29, Line 26:
> I'll take a look at that.
Indeed, it seems IWYU is confused, and I cannot see why this happens.

As a temporary workaround, please add path to this file into the 'muted'   in 
$KUDU_ROOT/build-support/iwyu/iwyu-filter.awk

...
  muted["kudu/hms/hms_client.cc"]
...

I'll deal with it a bit later.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 21 Oct 2017 00:35:43 +
Gerrit-HasComments: Yes

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

[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] KUDU-2191 (2/n): Hive Metastore client

[Alexey Serbin (Code Review)]

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

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 24:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/7053/24/src/kudu/hms/mini_hms.cc@101
PS24, Line 101:   map env_vars {
> Why should this variable in particular be const?
Since it's not intended to change in the scope, it might make sense to mark in 
const.  It's just a nit, feel free to ignore.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 24
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 21 Oct 2017 00:12:49 +
Gerrit-HasComments: Yes

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

[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

[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] KUDU-2191 (2/n): Hive Metastore client

[Alexey Serbin (Code Review)]

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

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 29:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.cc@26
PS29, Line 26:
> Yeah, IWYU's recommendations for this file don't make sense. Could you let
I'll take a look at that.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 21 Oct 2017 00:10:27 +
Gerrit-HasComments: Yes

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

[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

[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

[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

[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] KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager

[Todd Lipcon (Code Review)]

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

Change subject: KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager
..


Patch Set 1:

> Patch Set 1:
>
> > Patch Set 1:
> >
> > I dunno, I'm sort of leaning more towards the idea that true spin-locks 
> > should only be used for stuff as simple as "read this POD variable" and not 
> > for anything that involves acquiring other locks or allocating memory.
>
> The issue being that even if you address the acquisition sites I listed, we 
> still allocate memory while holding the TSTabletManager lock? I don't think 
> that's a huge deal, or, if it is, it's certainly a pattern we use widely 
> (i.e. take a spinlock, make a local copy of this map, release the spinlock).

Yea, I think we should probably move away from such patterns as much as we can, 
especially if the allocation is unbounded. If we're talking about allocating a 
4kb object it's most likely going to hit the local thread cache and be very 
fast, but I've found recently that allocations 1MB and larger can easily be 
tens or hundreds of milliseconds and in some cases even cross into >1s when 
things are under load.

Moving to an overall better lock implementation should reduce our cognitive 
load here quite a bit - a good lock like absl::Mutex should have the same 
performance as a spinlock but also be smart enough to go to sleep when 
necessary instead of spinning.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I763abddd74d8b1dabb618318dc84256b533077e3
Gerrit-Change-Number: 8345
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 20 Oct 2017 21:56:37 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager

[Adar Dembo (Code Review)]

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

Change subject: KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager
..


Patch Set 1:

> Patch Set 1:
>
> I dunno, I'm sort of leaning more towards the idea that true spin-locks 
> should only be used for stuff as simple as "read this POD variable" and not 
> for anything that involves acquiring other locks or allocating memory.

The issue being that even if you address the acquisition sites I listed, we 
still allocate memory while holding the TSTabletManager lock? I don't think 
that's a huge deal, or, if it is, it's certainly a pattern we use widely (i.e. 
take a spinlock, make a local copy of this map, release the spinlock).


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I763abddd74d8b1dabb618318dc84256b533077e3
Gerrit-Change-Number: 8345
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 20 Oct 2017 21:53:09 +
Gerrit-HasComments: No

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

[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

[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

[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

[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

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

[Adar Dembo (Code Review)]

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

Change subject: WIP 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 same?



--
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:05:24 +
Gerrit-HasComments: Yes

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

[Andrew Wong (Code Review)]

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

Change subject: WIP 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@1062
PS2, Line 1062: FinalizeBlock
> I don't think this will change much. MakeContainerAvailable already _doesn'
On second thought, maybe it's worth more consideration. Should the below 
bookkeeping not happen if it is read-only?



--
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 19:25:06 +
Gerrit-HasComments: Yes

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

[Andrew Wong (Code Review)]

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

Change subject: WIP 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@1062
PS2, Line 1062: FinalizeBlock
> Since Finalize() will call FinalizeBlock() no matter failure or success. I
I don't think this will change much. MakeContainerAvailable already _doesn't_ 
make it available if it's read-only. The main crux of the issue I think is when 
the block becomes read-only _after_ becoming available.



--
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 19:23:39 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

[Adar Dembo (Code Review)]

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

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
..


Patch Set 7:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/server/webserver.cc
File src/kudu/server/webserver.cc:

http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/server/webserver.cc@385
PS7, Line 385:   ScopedDisableRedaction s;
 :   if (kudu::g_should_redact_web) s.EnableRedaction();
This seems a little strange; how about just heap allocating the 
ScopedDisableRedaction if necessary and storing it in a unique_ptr? It's an 
extra allocation per web request, but it's very small and is 
allocated/deallocated by the same thread, so it should be fairly cheap.


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/server/webserver.cc@468
PS7, Line 468:   handler.callback()(req, );
Would it be sufficient to disable redaction around L468?


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flag_tags.h
File src/kudu/util/flag_tags.h:

http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flag_tags.h@84
PS7, Line 84: The values of these flags are considered sensitive and will be 
redacted
: // in the web UI and glog messages if --redact is set 
with 'all', or only in
: // glog messages if --redact is set with 'log'.
Let's be a little bit vague about this, to allow the redaction implementation 
to evolve: "The values of these flags are considered sensitive and will be 
redacted if redaction is enabled".


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.h
File src/kudu/util/flags.h:

http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.h@68
PS7, Line 68: // Get all the flags different from their defaults. The output is 
a nicely
: // formatted string with --flag=value pairs per line. Redact any 
flags that
: // are tagged as sensitive, if --redact is set with 'flag'.
Update.


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.cc
File src/kudu/util/flags.cc:

http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/flags.cc@545
PS7, Line 545: // Redact the flags tagged as sensitive, if --redact 
indicates
 : // that log messages should be redacted.
Seems like this call-site ought to check g_should_redact_log and the one in 
CommandlineFlagsIntoString should check g_should_redact_web.

What's confusing about all of this is that there's no guarantee that 
GetNonDefaultFlags is only used for logging or error messages, and that 
CommandlineFlagsIntoString is only used by the web UI. Maybe those functions 
should be renamed to indicate that purpose, so that if someone tries to 
repurpose them, they'll see that the redaction behavior should also be adjusted?


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h
File src/kudu/util/logging.h:

http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@63
PS7, Line 63: #define KUDU_SHOULD_REDACT() (kudu::g_should_redact_log && 
kudu::tls_redact_user_data)
Shouldn't this check g_should_redact_log || g_should_redact_web?


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@91
PS7, Line 91: extern bool g_should_redact_log;
It might be clearer if the tri-state --redact values were mapped to a tri-state 
enum instead of two bools. With two bools there's one state that doesn't make 
sense (g_should_redact_log=false && g_should_redact_web=true).


http://gerrit.cloudera.org:8080/#/c/6755/7/src/kudu/util/logging.h@93
PS7, Line 93: the
Nit: don't need the



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 20 Oct 2017 19:22:55 +
Gerrit-HasComments: Yes

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

[Hao Hao (Code Review)]

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

Change subject: WIP 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@1062
PS2, Line 1062: FinalizeBlock
Since Finalize() will call FinalizeBlock() no matter failure or success. I 
think it may be safe to check for read-only container here as well. As we do 
not want to make a container available if it is read-only.



--
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 19:21:29 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1957: Clarify web UI redaction in --redact flag help

[Hao Hao (Code Review)]

Hello Dan Burkert, Kudu Jenkins, Adar Dembo, Todd Lipcon,

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

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

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

Change subject: KUDU-1957: Clarify web UI redaction in --redact flag help
..

KUDU-1957: Clarify web UI redaction in --redact flag help

This patch refines the definition of --redact flag to control the
context of redaction. The available options now are 'all', 'log'
and 'none'. If 'all' is specified, sensitive data (sensitive
configuration flags and row data) will be redacted from the
web UI as well as glog and error messages. If 'log' is specified,
sensitive data will only be redacted from glog and error messages.
And the option 'flag' is removed from --redact flag to let this
flag only control redaction context.

Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
---
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/webserver-test.cc
M src/kudu/server/webserver.cc
M src/kudu/util/flag_tags-test.cc
M src/kudu/util/flag_tags.h
M src/kudu/util/flags-test.cc
M src/kudu/util/flags.cc
M src/kudu/util/flags.h
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/test_util.cc
12 files changed, 47 insertions(+), 32 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/6755/7
--
To view, visit http://gerrit.cloudera.org:8080/6755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Gerrit-Change-Number: 6755
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon