hawq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Roman Shaposhnik <ro...@shaposhnik.org>
Subject Re: [VOTE] HAWQ 2.0.0-incubating Release
Date Mon, 11 Jul 2016 16:33:19 GMT
On Mon, Jul 11, 2016 at 2:27 AM, Radar Da lei <rlei@pivotal.io> wrote:
> Hi Goden,
>
> I have pushed commits of 'HAWQ-892
> <https://issues.apache.org/jira/browse/HAWQ-892>' and 'HAWQ-901
> <https://issues.apache.org/jira/browse/HAWQ-901>' into branch
> '2.0.0.0-incubating'.

Ok, with these two additional commits I presumed the branch was ready
for review. I'm not done with the full review yet, but here are the top concerns
that would make me -1 this branch if it did go for a vote:
   0. mvn verify produces tons of RAT check failures that need to be carefully
   analyzed. As an aside -- I highly recommend having a CI job that
runs mvn verify
   on a regular basis.

   1. Pulling source from external repositories in an unconditional way.
    There's quite a bit of 'git clone' going on in the build system.
The easiest way
    to see it all is to run
       $ git grep -R 'git ' . | grep clone
    My first concern is that all of these calls need to be made
conditional. IOW,
    I should be able to build a basic HAWQ binary without it doing
'git clone' and
    instead relying on pointers to the same binary dependencies provided via
    build configuration. This could be a documentation issue and if so
I'd appreciate
    having it published on the wiki some place.

    On top of that, we have two bigger issues with the following repos:
        https://github.com/jconway/plr.git  -- GPL
        https://github.com/postgres/postgres.git -- Cryptography

    We need to make sure that HAWQ can be built with those altogether.'

    2. As a minor nit, I see that you imported thrift source under
depends/thirdparty/thrift
    and it would be great if there were a way to:
        2.1. make sure that it is obvious what *release* version of
thrift it was
        2.2. make sure that it is obvious if anything in there gets patched


Thanks,
Roman.

Mime
View raw message