Github user parthchandra commented on the issue:
https://github.com/apache/drill/pull/602
@laurentgo These changes are looking good. I just have the following
observations -
1) The build on Windows was a problem for me. I found CppUnit here
(http://dev-www.libreoffice.org/src/cppunit-1.13.2.tar.gz). There is a Windows
solutions file that will build on VS 2013
However FindCppUnit needs to be updated to find CppUnit on Windows as there
is no standard location the build will install to.
2) The cmake script now requires maven which in turn requires the JDK!. Is
it possible to generate the product version from the root pom? This would be
useful as the version is automatically changed at the time of the release.
Either way you should update the windows build instructions on where to get
CppUnit. Also, include instructions that the build requires JDK and Maven as
this was not previously required.
3) I hacked findCppUnit and that found the libs and header files, but the
unit tests failed to compile. I did not spend too much time on this though.
4) On MacOs, I upgraded my ZK libs to 3.4.7 (I built from source) and the
build goes thru fine. There are some new warnings which should be removed -
`/Users/pchandra/work/drill/contrib/native/client/src/clientlib/drillClientImpl.hpp:364:4:
warning: field 'm_bIsDirectConnection' will be initialized after field
'm_bIsConnected'
[-Wreorder]
m_bIsDirectConnection(false),
^
/Users/pchandra/work/drill/contrib/native/client/src/clientlib/drillClientImpl.hpp:583:34:
warning: field 'm_bIsDirectConnection' will be initialized after field
'm_maxConcurrentConnections' [-Wreorder]
PooledDrillClientImpl(): m_bIsDirectConnection(false),
m_maxConcurrentConnections(DEFAULT_MAX_CONCURRENT_CONNECTIONS),
m_lastConnection(-1), m_pError(NULL), m_quer...
^
/Users/pchandra/work/drill/contrib/native/client/src/clientlib/drillClientImpl.hpp:583:64:
warning: field 'm_maxConcurrentConnections' will be initialized after field
'm_lastConnection' [-Wreorder]
PooledDrillClientImpl(): m_bIsDirectConnection(false),
m_maxConcurrentConnections(DEFAULT_MAX_CONCURRENT_CONNECTIONS),
m_lastConnection(-1), m_pError(NULL), m_quer...
^
/Users/pchandra/work/drill/contrib/native/client/src/clientlib/drillClientImpl.hpp:583:150:
warning: field 'm_pError' will be initialized after field 'm_queriesExecuted'
[-Wreorder]
PooledDrillClientImpl(): m_bIsDirectConnection(false),
m_maxConcurrentConnections(DEFAULT_MAX_CONCURRENT_CONNECTIONS),
m_lastConnection(-1), m_pError(NULL), m_quer...
`
Other than that, things are really good. Even though it made reviewing the
code much harder, thank you for refactoring and cleaning up the code.
BTW, I also built and tested the build on linux, windows and macos. I also
ran a bunch of queries and cancels thru valgrind, and everything was nice and
clean.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
