Laszlo Gaal has posted comments on this change. ( http://gerrit.cloudera.org:8080/12996 )
Change subject: Enable reusing ccache directories. ...................................................................... Patch Set 3: Code-Review+1 (2 comments) Just a few points on readability http://gerrit.cloudera.org:8080/#/c/12996/3/functions.sh File functions.sh: http://gerrit.cloudera.org:8080/#/c/12996/3/functions.sh@519 PS3, Line 519: local TAR=ccache.tar Handling of the tarball filename is asymmetric between download_ccache() and upload_ccache(): it is abstracted here (maybe suggesting that it can be changed), but hardcoded in upload_ccache(). Would be easier to read if it followed the same convention. OTOH if there's a reason for the asymmetry, then a comment could clarify it. http://gerrit.cloudera.org:8080/#/c/12996/3/functions.sh@520 PS3, Line 520: S3_URL="https://native-toolchain Bucket name is hardcoded here, but configurable in upload_ccache(). Would be nice if it were symetrical. I realize that other places hardcode the bucket name too, but these two functions are obviously related. -- To view, visit http://gerrit.cloudera.org:8080/12996 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I482aa13e833d4680efe7cab98aad7f4fb998bfc0 Gerrit-Change-Number: 12996 Gerrit-PatchSet: 3 Gerrit-Owner: Hector Acosta <hector.aco...@cloudera.com> Gerrit-Reviewer: Laszlo Gaal <laszlo.g...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Tue, 16 Apr 2019 21:56:45 +0000 Gerrit-HasComments: Yes
