[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-06-05 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13487 )

Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..


Patch Set 5: Verified+1


--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 05 Jun 2019 23:40:29 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-06-05 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/13487 )

Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..

IMPALA-8425: part 1: reduce size of binaries in container

* Symlink impalad/catalog/statestored inside container.
  This doesn't seem to really save any space - there's
  some kind of deduplication going on.
* Don't include libfesupport.so, which shouldn't be needed.
* strip debug symbols from the binary.
* Only include the libkuduclient.so libraries for Kudu

This shaves ~1.1GB from the image size- 250MB as a result
of the impalad binary changes and the remainder from the
Kudu changs.

Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Reviewed-on: http://gerrit.cloudera.org:8080/13487
Reviewed-by: Impala Public Jenkins 
Tested-by: Impala Public Jenkins 
---
M docker/daemon_entrypoint.sh
M docker/impala_base/Dockerfile
M docker/setup_build_context.py
3 files changed, 34 insertions(+), 23 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 


[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-06-05 Thread Joe McDonnell (Code Review)
Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13487 )

Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13487/4/docker/impala_base/Dockerfile
File docker/impala_base/Dockerfile:

http://gerrit.cloudera.org:8080/#/c/13487/4/docker/impala_base/Dockerfile@30
PS4, Line 30: # Copy build artifacts required for the daemon processes.
: # Need to have multiple copy commands to preserve directory 
structure.
: COPY lib /opt/impala/lib
: COPY www /opt/impala/www
: COPY bin /opt/impala/bin
: # Symlink here instead of in setup_build_context to avoid 
duplicate binaries.
: RUN cd /opt/impala/bin && ln -s impalad statestored && ln -s 
impalad catalogd && \
: # Create conf directory for later config injection.
: mkdir /opt/impala/conf && \
: # Create logs directory to collect container logs.
: mkdir /opt/impala/logs
> The issue was that COPY runs as root, so the destination files end up being
Ah, interesting, that makes sense.



--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 05 Jun 2019 18:15:16 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-06-05 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13487 )

Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4416/ 
DRY_RUN=false


--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 05 Jun 2019 18:09:11 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-06-05 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13487 )

Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..


Patch Set 5: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 05 Jun 2019 18:09:10 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-06-05 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13487 )

Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13487/4/docker/impala_base/Dockerfile
File docker/impala_base/Dockerfile:

http://gerrit.cloudera.org:8080/#/c/13487/4/docker/impala_base/Dockerfile@30
PS4, Line 30: # Copy build artifacts required for the daemon processes.
: # Need to have multiple copy commands to preserve directory 
structure.
: COPY lib /opt/impala/lib
: COPY www /opt/impala/www
: COPY bin /opt/impala/bin
: # Symlink here instead of in setup_build_context to avoid 
duplicate binaries.
: RUN cd /opt/impala/bin && ln -s impalad statestored && ln -s 
impalad catalogd && \
: # Create conf directory for later config injection.
: mkdir /opt/impala/conf && \
: # Create logs directory to collect container logs.
: mkdir /opt/impala/logs
> I was thinking about the block of COPYs and the RUN that makes some directo
The issue was that COPY runs as root, so the destination files end up being 
owned by root. And if we do that after switching to impala, we can't modify the 
files.



--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 05 Jun 2019 18:08:59 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-06-05 Thread Joe McDonnell (Code Review)
Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13487 )

Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..


Patch Set 4: Code-Review+2

(1 comment)

> For our test runs we would still have the unstripped binary
 > available (I guess I hadn't confirmed that worked correctly). I
 > don't know about the more general question of whether we want to
 > optionally provide containers with the symbols built-in. It does
 > seem like a lot of overhead to the size of the containers, and you
 > would still have to extract the binary from the container to use it
 > with GDB and a core file. It's nice that that would be an option
 > though.
 >
 > We still should have enough symbols to get backtraces as-is.

That makes sense. In the test, we have the original binary, so at the end 
bin/jenkins/finalize.sh can resolve the minidumps. For things that end up in an 
actual deployment, we'll want to preserve the symbols or original binary 
somewhere, but that is not a new problem. The tests were the main thing I was 
concerned about.

http://gerrit.cloudera.org:8080/#/c/13487/4/docker/impala_base/Dockerfile
File docker/impala_base/Dockerfile:

http://gerrit.cloudera.org:8080/#/c/13487/4/docker/impala_base/Dockerfile@30
PS4, Line 30: # Copy build artifacts required for the daemon processes.
: # Need to have multiple copy commands to preserve directory 
structure.
: COPY lib /opt/impala/lib
: COPY www /opt/impala/www
: COPY bin /opt/impala/bin
: # Symlink here instead of in setup_build_context to avoid 
duplicate binaries.
: RUN cd /opt/impala/bin && ln -s impalad statestored && ln -s 
impalad catalogd && \
: # Create conf directory for later config injection.
: mkdir /opt/impala/conf && \
: # Create logs directory to collect container logs.
: mkdir /opt/impala/logs
> Yeah it does - after USER impala, the commands are run as the Impala user,
I was thinking about the block of COPYs and the RUN that makes some directories 
and links executables. It moved from after USER impala to before.

None of that impacts the review, they both seem correct. I'm just curious if 
there was anything going on under the covers.



--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 05 Jun 2019 16:43:02 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-06-04 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13487 )

Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13487/4/docker/impala_base/Dockerfile
File docker/impala_base/Dockerfile:

http://gerrit.cloudera.org:8080/#/c/13487/4/docker/impala_base/Dockerfile@30
PS4, Line 30: # Copy build artifacts required for the daemon processes.
: # Need to have multiple copy commands to preserve directory 
structure.
: COPY lib /opt/impala/lib
: COPY www /opt/impala/www
: COPY bin /opt/impala/bin
: # Symlink here instead of in setup_build_context to avoid 
duplicate binaries.
: RUN cd /opt/impala/bin && ln -s impalad statestored && ln -s 
impalad catalogd && \
: # Create conf directory for later config injection.
: mkdir /opt/impala/conf && \
: # Create logs directory to collect container logs.
: mkdir /opt/impala/logs
> Curious: does it matter whether this is before or after the USER impala set
Yeah it does - after USER impala, the commands are run as the Impala user, 
which doesn't have permissions to change ownership of root-owned directories.



--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 05 Jun 2019 00:46:08 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-06-04 Thread Tim Armstrong (Code Review)
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13487 )

Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..


Patch Set 4:

For our test runs we would still have the unstripped binary available (I guess 
I hadn't confirmed that worked correctly). I don't know about the more general 
question of whether we want to optionally provide containers with the symbols 
built-in. It does seem like a lot of overhead to the size of the containers, 
and you would still have to extract the binary from the container to use it 
with GDB and a core file. It's nice that that would be an option though.

We still should have enough symbols to get backtraces as-is.


--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 05 Jun 2019 00:45:16 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-06-04 Thread Joe McDonnell (Code Review)
Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13487 )

Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..


Patch Set 4:

(1 comment)

This makes sense to me. Thanks for incorporating the Kudu change.

>From a testing perspective, do we want to strip debug symbols? These tests 
>shouldn't crash all that often, but when they do, symbols are nice.

http://gerrit.cloudera.org:8080/#/c/13487/4/docker/impala_base/Dockerfile
File docker/impala_base/Dockerfile:

http://gerrit.cloudera.org:8080/#/c/13487/4/docker/impala_base/Dockerfile@30
PS4, Line 30: # Copy build artifacts required for the daemon processes.
: # Need to have multiple copy commands to preserve directory 
structure.
: COPY lib /opt/impala/lib
: COPY www /opt/impala/www
: COPY bin /opt/impala/bin
: # Symlink here instead of in setup_build_context to avoid 
duplicate binaries.
: RUN cd /opt/impala/bin && ln -s impalad statestored && ln -s 
impalad catalogd && \
: # Create conf directory for later config injection.
: mkdir /opt/impala/conf && \
: # Create logs directory to collect container logs.
: mkdir /opt/impala/logs
Curious: does it matter whether this is before or after the USER impala setting?



--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 05 Jun 2019 00:34:19 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-05-31 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13487 )

Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..


Patch Set 3: Verified+1


--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 31 May 2019 22:32:55 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-05-31 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13487 )

Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..


Patch Set 3:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/3466/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 31 May 2019 20:25:33 +
Gerrit-HasComments: No


[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-05-31 Thread Tim Armstrong (Code Review)
Hello Joe McDonnell, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/13487

to look at the new patch set (#4).

Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..

IMPALA-8425: part 1: reduce size of binaries in container

* Symlink impalad/catalog/statestored inside container.
  This doesn't seem to really save any space - there's
  some kind of deduplication going on.
* Don't include libfesupport.so, which shouldn't be needed.
* strip debug symbols from the binary.
* Only include the libkuduclient.so libraries for Kudu

This shaves ~1.1GB from the image size- 250MB as a result
of the impalad binary changes and the remainder from the
Kudu changs.

Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
---
M docker/daemon_entrypoint.sh
M docker/impala_base/Dockerfile
M docker/setup_build_context.py
3 files changed, 34 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/13487/4
--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 


[Impala-ASF-CR] IMPALA-8425: part 1: reduce size of binaries in container

2019-05-31 Thread Tim Armstrong (Code Review)
Tim Armstrong has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/13487


Change subject: IMPALA-8425: part 1: reduce size of binaries in container
..

IMPALA-8425: part 1: reduce size of binaries in container

* Symlink impalad/catalog/statestored inside container.
  This doesn't seem to really save any space - there's
  some kind of deduplication going on.
* Don't include libfesupport.so, which shouldn't be needed.
* strip debug symbols from the binary.
* Only include the libkuduclient.so libraries for Kudu

This shaves ~1.1GB from the image size- 250MB as a result
of the impalad binary changes and the remainder from the
Kudu changs.

Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
---
M docker/daemon_entrypoint.sh
M docker/impala_base/Dockerfile
M docker/setup_build_context.py
3 files changed, 32 insertions(+), 22 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/13487/3
--
To view, visit http://gerrit.cloudera.org:8080/13487
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I95ff479bedd3b93e6569e72f03f42acd9dba8b14
Gerrit-Change-Number: 13487
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong