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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
