drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jacques-n <...@git.apache.org>
Subject [GitHub] drill pull request: DRILL-3987: Componentize Drill, extracting vec...
Date Wed, 11 Nov 2015 00:57:02 GMT
Github user jacques-n commented on the pull request:

    https://github.com/apache/drill/pull/250#issuecomment-155617242
  
    It seems like there are two points that Chris is asking about:
    
    1) The scale of this patch (is it too big?)
    2) The order of application between this patch and other patches (such as the allocator
patch)
    
    On (1): other than moving files, there are virtually no functional changes in this patch
and its purpose is extremely narrow in scope. (There are probably six or so separations between
interface and implementation.) Yes, there are a lot of files that were moved. The patch creates
five new modules and has to move the appropriate files into those directories. All changes
were focused on the same purpose of modularization. I also chose to limit any refactoring
required in these patches exactly to avoid this challenge. (Note the orange lines that I deferred
in the attached PPT). As such, I think it is unfair to characterize a modularization patch
by counting the number of files that were involved. 
    
    On (2): We typically order things in the order that they are ready to be merged. If this
is a reasonable patch and is ready to be merged, it should be merged. If another patch has
been reviewed, passes functional (and performance regression as appropriate) and is ready
to be merged, it should be. In general, patches that are ready are ahead of patches that aren't
ready.
    
    Specifically on the allocator patch, on numerous occasions we have held up commits to
master (and releases) to try get the allocator patch merged. On each occasion, we've found
that it isn't ready to be merged due to concurrency design challenges. Let's address those
before worrying about staging of the patch. Once we get the algorithm right and everyone has
signed off on that, we can figure out rebasing on master and staging in relationship to other
patches. 


---
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