-1 (binding) Sorry my review is so late; I know that you’ve already closed the vote; but I figured my feedback would be more useful now than waiting for RC6.
The release is actually in good shape; well done. My formal “-1” vote is due to issue 0 (content of the vote email) and 5 (copyright year). Please fix these show-stoppers in the next RC. Other issues are well worth looking into, because others may raise them in the IPMC vote. Julian 0. I found the release candidate at https://dist.apache.org/repos/dist/dev/incubator/quickstep/0.1.0/RC5/ <https://dist.apache.org/repos/dist/dev/incubator/quickstep/0.1.0/RC5/>. Please include its URL and its hashes (the URLs of the hashes AND the actual hash values) in the vote email next time. The proposition to be voted upon needs to be self-contained and immutable. 1. KEYS [ https://dist.apache.org/repos/dist/dev/incubator/quickstep/KEYS <https://dist.apache.org/repos/dist/dev/incubator/quickstep/KEYS> ] looks a bit strange — usually the user & signers are at the top of each block (see for example http://www.apache.org/dist/calcite/KEYS <http://www.apache.org/dist/calcite/KEYS>) but import succeeded: $ gpg --import KEYS gpg: key A3C29E12: public key "Jignesh Patel <jign...@apache.org>" imported gpg: key 5A29899A: public key "Marc Spehlmann (apache) <sp...@apache.org>" imported gpg: Total number processed: 2 gpg: imported: 2 (RSA: 2) 2. Checked hashes and keys. (Let’s move to something stronger than MD5 and SHA1 soon. Also, let’s do a key signing party so that Marc & Jignesh are in the web of trust.) 3. The tar-ball contains KEYS. Not much point in that (because if someone compromised the tar-ball they would undoubtedly insert their a compromised KEYS file!), but no harm either. 4. I didn’t try to build (I’m not on a machine with sufficient horse-power), but it’s good to see clear build instructions in the tar-ball. 5. Checked that LICENSE, DISCLAIMER, NOTICE. The year in NOTICE must be 2017. 6. Are all of the lines in NOTICE *required*? People tend to put lines into a NOTICE out of *courtesy*. But it is mis-placed courtesy because the consumers of your software are *required* to reproduce your NOTICE verbatim. If Quickstep Technologies and Pivotal Software contributed under the ASL, then I don’t believe their copyright notices are required. In NOTICE, by “third_party/benchmark”, do you mean “third_party/src/tmb/benchmarks”? third_party/src/tmb/README.md says "TMB is part of the Quickstep project (copyright Pivotal Software, Inc.) and is distributed under the same license terms.” This statement is confusing now that Quickstep is now Apache Quickstep. If TMB has been contributed to Apache Quickstep can we just move it out of third_party? third_party/linenoise and third_party/gpertools don’t seem to be included in the tar-ball, so the entry can be removed from NOTICE. 7. It is useful (not required) to include release notes, including changes since the previous release. Some projects include a HISTORY.md file. 8. There are pivotalsoftware.com URLs in README.md and DEV_README.md. Can you change these to apache.org <http://apache.org/>. 9. Apache Rat found the following files without headers (ignoring files under third_party/src): ./.gitattributes ./.gitignore ./.gitmodules ./.travis.yml ./BUILDING.md ./CREDITS.md ./DEV_README.md ./Doxyfile ./GENERATOR_FUNCTIONS.md ./README.md ./WORKING_WITH_AN_IDE.md ./build/vagrant/README.md ./parser/preprocessed/SqlLexer_gen.hpp ./parser/preprocessed/genfiles.sh ./query_execution/README.md ./relational_operators/tests/text_scan_faulty_golden_output.txt ./relational_operators/tests/text_scan_faulty_input.txt ./relational_operators/tests/text_scan_golden_output.txt ./relational_operators/tests/text_scan_input.txt ./third_party/README.md ./third_party/download_and_patch_prerequisites.sh ./third_party/reset_third_party_dir.sh ./third_party/patches/benchmark/benchmarkCMake.patch ./third_party/patches/benchmark/benchmarkSrcCMakeLists.patch ./third_party/patches/gflags/CMakeLists.patch ./third_party/patches/gflags/gflags_reporting.cc.patch ./third_party/patches/linenoise/CMakeLists.txt ./third_party/patches/linenoise/linenoise.c.patch ./third_party/patches/linenoise/linenoise.h.patch ./third_party/patches/re2/re2CMake.patch ./types/README.md Add headers to as many of these as you can. 10. Apache Rat found 2 files that it considered to be GPL3: GPL3 ./parser/preprocessed/SqlParser_gen.cpp GPL3 ./parser/preprocessed/SqlParser_gen.hpp But according to https://issues.apache.org/jira/browse/LEGAL-291 <https://issues.apache.org/jira/browse/LEGAL-291> these files can be distributed under ASL. So we’re good. Julian > On Mar 2, 2017, at 12:25 PM, Harshad Deshmukh <hars...@cs.wisc.edu> wrote: > > Hi Marc, > > +1 from me. Here's a brief summary of what I tested - > > 1. Download the source code - OK > > 2. Download the third party libraries (dependencies) - OK > > 3. cmake build - OK > > 4. Source code build - OK > > 5. Running tests - OK. > > > On 03/02/2017 11:48 AM, Jignesh Patel wrote: >> +1 from me! >> >> On 3/1/17, 10:48 PM, "Jianqiao" <jianq...@apache.org> wrote: >> >> +1. I'm good with the release. >> Best, >> Jianqiao >> 2017-03-01 17:21 GMT-06:00 HAKAN MEMISOGLU <memiso...@cs.wisc.edu>: >> > Hi Marc, >> > >> > +1 from me. It works with Docker. >> > >> > However I also get an error when I try to build it with Mac. >> > The linker cannot find some symbols that protoc uses. >> > >> > > On Mar 1, 2017, at 4:30 PM, Marc Spehlmann <spehl.apa...@gmail.com> >> > wrote: >> > > >> > > Jianqiao and I worked today to merge some PRs that we thought should >> go >> > in >> > > the first release. Accordingly we have created another candidate. >> > > >> > > As per Apache, *we require 3 +1 votes* from project members to make >> this >> > an >> > > official release. >> > > >> > > Before voting, please test the release (build, run ctest). For a >> guide on >> > > how to test, GOTO release/README.md >> > > >> > > This vote will remain open for 72 hours or until we find issues that >> we >> > can >> > > quickly correct and create another candidate. >> > > >> > > more details on >> > > >> > > https://cwiki.apache.org/confluence/display/QUICKSTEP/How+To+Release >> > > >> > > --Marc >> > >> > >> >> > > -- > Thanks, > Harshad >
