Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17974 )
Change subject: [encryption] KUDU-3331 Encrypt file system ...................................................................... Patch Set 8: (10 comments) http://gerrit.cloudera.org:8080/#/c/17974/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17974/7//COMMIT_MSG@32 PS7, Line 32: be us > nit: seems to be an extra word? Done http://gerrit.cloudera.org:8080/#/c/17974/7//COMMIT_MSG@45 PS7, Line 45: tes. All tests below wer > Just to double-check: the results below are for RELEASE builds, right? Yes, added a note about it. http://gerrit.cloudera.org:8080/#/c/17974/7//COMMIT_MSG@133 PS7, Line 133: I also wanted to run dense_node-itest with -num_seconds=240 and > Actually I misunderstood the design of this test: to maximize the number of It seems the results are all over the place even with rowset compaction disabled. Do you think I should just remove this part from the commit message? http://gerrit.cloudera.org:8080/#/c/17974/7//COMMIT_MSG@134 PS7, Line 134: but the amount of data written (both in terms of : number of blocks and bytes) was a > nit: missing some words? Done http://gerrit.cloudera.org:8080/#/c/17974/7/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/17974/7/src/kudu/fs/log_block_manager-test.cc@a1173 PS7, Line 1173: : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : > Was this removal intentional? Nope, thanks. http://gerrit.cloudera.org:8080/#/c/17974/6/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/17974/6/src/kudu/tserver/tablet_copy_client.cc@492 PS6, Line 492: > What about explicitly controlling the encryption for the result file here? Done http://gerrit.cloudera.org:8080/#/c/17974/6/src/kudu/tserver/tablet_copy_client.cc@798 PS6, Line 798: > What about explicitly controlling the encryption for the result file here? Done http://gerrit.cloudera.org:8080/#/c/17974/7/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/17974/7/src/kudu/util/env_posix.cc@203 PS7, Line 203: > nit: since now the terminology is more stable (encrypted vs sensitive, etc. Done http://gerrit.cloudera.org:8080/#/c/17974/7/src/kudu/util/env_posix.cc@1306 PS7, Line 1306: > nit: would it make sense to introduce a helper function/method ShouldEncryp I don't know about that, each file type uses separate *FileOptions types, so the helpers would need to be duplicated. Or should I use templates for this? http://gerrit.cloudera.org:8080/#/c/17974/6/src/kudu/util/rolling_log.cc File src/kudu/util/rolling_log.cc: http://gerrit.cloudera.org:8080/#/c/17974/6/src/kudu/util/rolling_log.cc@163 PS6, Line 163: opts.is_sensitive = false; > Just to make sure this assignment is left here intentionally even if 'is_se It's not necessarily needed, but it doesn't hurt. Also, it makes things simpler if down the line it turns out it's easier/better to have is_sensitive default to true. -- To view, visit http://gerrit.cloudera.org:8080/17974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I909d0c4af0c1fca0d14c99a6627842dbe2ed7524 Gerrit-Change-Number: 17974 Gerrit-PatchSet: 8 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 30 Nov 2021 18:50:30 +0000 Gerrit-HasComments: Yes
