drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From parthchandra <...@git.apache.org>
Subject [GitHub] drill issue #602: Improve Drill C++ connector
Date Wed, 19 Oct 2016 23:27:32 GMT
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message