Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22599 )
Change subject: IMPALA-13795: Support serving webUI metadata with gzip compression ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/22599/1//COMMIT_MSG Commit Message: PS1: Does it make sense to add an automated test to verify that the server sends out gzip-compressed message when appropriate header is present and send plain content when it's not? That's to catch future regressions, if any. http://gerrit.cloudera.org:8080/#/c/22599/1/be/src/util/compress.h File be/src/util/compress.h: http://gerrit.cloudera.org:8080/#/c/22599/1/be/src/util/compress.h@66 PS1, Line 66: int compression_level_ nit: could this be 'const'? http://gerrit.cloudera.org:8080/#/c/22599/1/be/src/util/compress.cc File be/src/util/compress.cc: http://gerrit.cloudera.org:8080/#/c/22599/1/be/src/util/compress.cc@49 PS1, Line 49: bzero(&stream_, sizeof(stream_)); nit: consider adding DCHECK() for compression_level; I guess it should be between Z_DEFAULT_COMPRESSION (-1) and Z_BEST_COMPRESSION (9), inclusive http://gerrit.cloudera.org:8080/#/c/22599/1/be/src/util/webserver.cc File be/src/util/webserver.cc: http://gerrit.cloudera.org:8080/#/c/22599/1/be/src/util/webserver.cc@257 PS1, Line 257: compressor->Close(); Is it possible to call this even if compressor->ProcessBlock() above fails? E.g., call Close() in the destructor of some sort of ScopedCleanup object? http://gerrit.cloudera.org:8080/#/c/22599/1/be/src/util/webserver.cc@293 PS1, Line 293: CompressStringToBuffer(content, output); What happens if this fails? Will the result content be sent corrupted? Maybe, consider changing the signature of CompressStringToBuffer and the logic to send out just plain data if the compression fails. Also, I'd think of writing a log line about failed compression attempt, keeping throttling of log messages in mind to avoid flooding the logs. -- 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: 1 Gerrit-Owner: Surya Hebbar <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Fri, 07 Mar 2025 23:27:29 +0000 Gerrit-HasComments: Yes
