Surya Hebbar has posted comments on this change. ( http://gerrit.cloudera.org:8080/22599 )
Change subject: IMPALA-13795: Support serving webUI content with gzip compression ...................................................................... Patch Set 2: > I didn't pay attention to this in my first review round, but after Riza added > his comment, I took another look. > Why to change this in the scope of this patch at all? Reading the > description of IMPALA-13795, I don't see how this is relevant. The main reason for adding this was to support Z_BEST_SPEED(1), as the default Z_DEFAULT_STRATEGY(-1) uses 6 as the default compression level, which would be slower for serving over the network. Please let me know if there is another method to circumvent this. I had initially thought it would be simple, but the default initialization of 0 with CodecInfo seems to have many hardcoded values or dependent initializations based on it and even tests also had default compression levels as 0. > I'd suggest to address this in a separate changelist instead of sneaking it > into this patch. This update might have unintended consequences, and if you > changed it, then I'd expect you provide adequate testing coverage. A proper > venue for that would be a separate changelist, with corresponding tests > coverage added, IMO. It's a good practice to address logically separate > items in different patches instead of piling everything into a single patch. > CodecInfo's compression_level_ defaults to 0, which would turn off > compression for zlib. I've toyed with compression levels for other > compression codecs like snappy/lz4 in the past, and I ended up adding the > concept of UNSET_COMPRESSION_LEVEL which I set to INTMAXVAL. Zero can be a > valid compression level, but INTMAXVAL won't be. Individual codecs can detect > UNSET_COMPRESSION_LEVEL and use the default. > I agree with Alexey that this change seems unnecessary for the webserver > change. Thank you for the for the comments, they were very helpful in understanding the scope of these changes. I have posted a patch in initial stages which have passed the DecompressorTest.*, and have created IMPALA-13923, but need to add further information to the JIRA and coverage for the proposed changes. I did not see a method to use compression levels in GZIP without making these changes in IMPALA-13923, please let me know, if this can be achieved in any other simple methods, which I am missing. -- To view, visit http://gerrit.cloudera.org:8080/22599 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I431088a30337bbef2c8d6e16dd15fb6572db0f15 Gerrit-Change-Number: 22599 Gerrit-PatchSet: 2 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Tue, 01 Apr 2025 09:04:16 +0000 Gerrit-HasComments: No
