Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15454 )
Change subject: IMPALA-3766: optionally compress spilled data ...................................................................... Patch Set 9: (5 comments) went through this briefly, few comments http://gerrit.cloudera.org:8080/#/c/15454/9/be/src/runtime/bufferpool/buffer-pool-counters.h File be/src/runtime/bufferpool/buffer-pool-counters.h: http://gerrit.cloudera.org:8080/#/c/15454/9/be/src/runtime/bufferpool/buffer-pool-counters.h@59 PS9, Line 59: RuntimeProfile::Counter* uncompressed_bytes_written; why add this here vs the TmpFileMgr? http://gerrit.cloudera.org:8080/#/c/15454/9/be/src/runtime/tmp-file-mgr-internal.h File be/src/runtime/tmp-file-mgr-internal.h: http://gerrit.cloudera.org:8080/#/c/15454/9/be/src/runtime/tmp-file-mgr-internal.h@96 PS9, Line 96: int64_t bytes_allocated_ = 0; would it be clearer to rename this to 'file_write_offset_' http://gerrit.cloudera.org:8080/#/c/15454/9/be/src/runtime/tmp-file-mgr.h File be/src/runtime/tmp-file-mgr.h: http://gerrit.cloudera.org:8080/#/c/15454/9/be/src/runtime/tmp-file-mgr.h@157 PS9, Line 157: static constexpr int64_t HOLE_PUNCH_BLOCK_SIZE = 4096; nit: docs on what this is used for and why it is set to 4K http://gerrit.cloudera.org:8080/#/c/15454/9/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: http://gerrit.cloudera.org:8080/#/c/15454/9/be/src/runtime/tmp-file-mgr.cc@61 PS9, Line 61: "most cases this should be used in conjunction with --disk_spill_punch_holes=true " : "to maximize the space saved."); should we just enforce this? e.g. compression can only be used if hole punching is support on the underlying fs. if users complain and start asking for spill compression support on a fs without hole punching, we can always remove the restriction later. i think the benefit is that is simplifies the combination of configurations Impala needs to support http://gerrit.cloudera.org:8080/#/c/15454/9/be/src/runtime/tmp-file-mgr.cc@63 PS9, Line 63: disk_spill_compression_buffer_limit_bytes what happens if compressing a buffer requires more than this many bytes? -- To view, visit http://gerrit.cloudera.org:8080/15454 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c08ff9504097f0fee8c32316c5c150136abe659 Gerrit-Change-Number: 15454 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 23 Mar 2020 21:01:24 +0000 Gerrit-HasComments: Yes
