You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by sohami <gi...@git.apache.org> on 2017/08/22 18:18:53 UTC
[GitHub] drill pull request #919: Drill 5721
GitHub user sohami opened a pull request:
https://github.com/apache/drill/pull/919
Drill 5721
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sohami/drill DRILL-5721
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/919.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #919
----
commit 64f85c893020da5fda12fb27635321326dfb544f
Author: Sorabh Hamirwasia <sh...@maprtech.com>
Date: 2017-08-17T23:48:37Z
DRILL-5721: Refactor for FragmentManager, setupRootFragment and startNewFragment
commit 7a004688510862e73785ba275e6173fcd4dba344
Author: Sorabh Hamirwasia <sh...@maprtech.com>
Date: 2017-08-18T22:00:10Z
DRILL-5721: Query with only root fragment and no non-root fragment hangs when Drillbit to Drillbit Control Connection has network issues
Note: To resolve the issue all the fragments including root fragment which are assigned to be executed on Foreman node are scheduled locally
and not sent over Control Tunnel. Also the FragmentStatusReporter is updated to sent the status update locally by fragments running on
Foreman node.
----
---
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.
---
[GitHub] drill issue #919: DRILL-5721: Query with only root fragment and no non-root ...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/919
@paul-rogers - Not sure which commit you are referring to. All the commits are reviewed by @parthchandra. I just rebased commits to latest master after last week pull request since @amansinha100 was observing some random functional test failure. After rebase I ran those tests in isolation for multiple iterations and those were passing. Let me know if you want me to combine all these 3 commits.
---
[GitHub] drill pull request #919: DRILL-5721: Query with only root fragment and no no...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/919
---
[GitHub] drill pull request #919: DRILL-5721: Query with only root fragment and no no...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/919#discussion_r135949946
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -1109,7 +1102,7 @@ private void setupNonRootFragments(final Collection<PlanFragment> fragments) thr
// nothing to do here
return;
}
- /*
+ /*z
--- End diff --
It was part for refactor commit only and was later removed in next commit.
---
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.
---
[GitHub] drill pull request #919: DRILL-5721: Query with only root fragment and no no...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/919#discussion_r136157876
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -1073,26 +1070,22 @@ public QueryId getQueryId() {
*/
private void setupRootFragment(final PlanFragment rootFragment, final FragmentRoot rootOperator)
throws ExecutionSetupException {
- @SuppressWarnings("resource")
final FragmentContext rootContext = new FragmentContext(drillbitContext, rootFragment, queryContext,
initiatingClient, drillbitContext.getFunctionImplementationRegistry());
- @SuppressWarnings("resource")
- final IncomingBuffers buffers = new IncomingBuffers(rootFragment, rootContext);
- rootContext.setBuffers(buffers);
-
- queryManager.addFragmentStatusTracker(rootFragment, true);
-
final ControlTunnel tunnel = drillbitContext.getController().getTunnel(queryContext.getCurrentEndpoint());
+ final FragmentStatusReporter statusReporter = new FragmentStatusReporter(rootContext, tunnel);
final FragmentExecutor rootRunner = new FragmentExecutor(rootContext, rootFragment,
- new FragmentStatusReporter(rootContext, tunnel),
- rootOperator);
- final RootFragmentManager fragmentManager = new RootFragmentManager(rootFragment.getHandle(), buffers, rootRunner);
+ statusReporter, rootOperator);
- if (buffers.isDone()) {
+ queryManager.addFragmentStatusTracker(rootFragment, true);
+
+ // FragmentManager is setting buffer for FragmentContext
+ if (rootContext.isBuffersDone()) {
--- End diff --
Sorry about the confusion. It was moved out of else in second commit.
---
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.
---
[GitHub] drill pull request #919: DRILL-5721: Query with only root fragment and no no...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/919#discussion_r136155556
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -1073,26 +1070,22 @@ public QueryId getQueryId() {
*/
private void setupRootFragment(final PlanFragment rootFragment, final FragmentRoot rootOperator)
throws ExecutionSetupException {
- @SuppressWarnings("resource")
final FragmentContext rootContext = new FragmentContext(drillbitContext, rootFragment, queryContext,
initiatingClient, drillbitContext.getFunctionImplementationRegistry());
- @SuppressWarnings("resource")
- final IncomingBuffers buffers = new IncomingBuffers(rootFragment, rootContext);
- rootContext.setBuffers(buffers);
-
- queryManager.addFragmentStatusTracker(rootFragment, true);
-
final ControlTunnel tunnel = drillbitContext.getController().getTunnel(queryContext.getCurrentEndpoint());
+ final FragmentStatusReporter statusReporter = new FragmentStatusReporter(rootContext, tunnel);
final FragmentExecutor rootRunner = new FragmentExecutor(rootContext, rootFragment,
- new FragmentStatusReporter(rootContext, tunnel),
- rootOperator);
- final RootFragmentManager fragmentManager = new RootFragmentManager(rootFragment.getHandle(), buffers, rootRunner);
+ statusReporter, rootOperator);
- if (buffers.isDone()) {
+ queryManager.addFragmentStatusTracker(rootFragment, true);
+
+ // FragmentManager is setting buffer for FragmentContext
+ if (rootContext.isBuffersDone()) {
--- End diff --
Yes but from the looks of it the fragment manager hasn't been created yet. In fact it seems it is created in the else part of the condition you're checking.
---
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.
---
[GitHub] drill issue #919: DRILL-5721: Query with only root fragment and no non-root ...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/919
Rebased on latest master and squashed the initial 3 commits. But I have kept the commit to resolve conflict separate as there are some changes made w.r.t DRILL-3449 behavior, and added some new unit tests.
---
[GitHub] drill issue #919: DRILL-5721: Query with only root fragment and no non-root ...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/919
Attempt to merge into master failed:
```
CONFLICT (content): Merge conflict in exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentStatusReporter.java
```
Please rebase onto latest master. While at it, please also squash commits.
---
[GitHub] drill issue #919: DRILL-5721: Query with only root fragment and no non-root ...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/919
Rebased the branch to latest master and updated test's included in DRILL-5701
---
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.
---
[GitHub] drill pull request #919: DRILL-5721: Query with only root fragment and no no...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/919#discussion_r135937433
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -1073,26 +1070,22 @@ public QueryId getQueryId() {
*/
private void setupRootFragment(final PlanFragment rootFragment, final FragmentRoot rootOperator)
throws ExecutionSetupException {
- @SuppressWarnings("resource")
final FragmentContext rootContext = new FragmentContext(drillbitContext, rootFragment, queryContext,
initiatingClient, drillbitContext.getFunctionImplementationRegistry());
- @SuppressWarnings("resource")
- final IncomingBuffers buffers = new IncomingBuffers(rootFragment, rootContext);
- rootContext.setBuffers(buffers);
-
- queryManager.addFragmentStatusTracker(rootFragment, true);
-
final ControlTunnel tunnel = drillbitContext.getController().getTunnel(queryContext.getCurrentEndpoint());
+ final FragmentStatusReporter statusReporter = new FragmentStatusReporter(rootContext, tunnel);
final FragmentExecutor rootRunner = new FragmentExecutor(rootContext, rootFragment,
- new FragmentStatusReporter(rootContext, tunnel),
- rootOperator);
- final RootFragmentManager fragmentManager = new RootFragmentManager(rootFragment.getHandle(), buffers, rootRunner);
+ statusReporter, rootOperator);
- if (buffers.isDone()) {
+ queryManager.addFragmentStatusTracker(rootFragment, true);
+
+ // FragmentManager is setting buffer for FragmentContext
+ if (rootContext.isBuffersDone()) {
--- End diff --
I don't see where rootContext.setBuffers is being called to set the buffers
---
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.
---
[GitHub] drill issue #919: DRILL-5721: Query with only root fragment and no non-root ...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on the issue:
https://github.com/apache/drill/pull/919
+1 LGTM
---
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.
---
[GitHub] drill pull request #919: DRILL-5721: Query with only root fragment and no no...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/919#discussion_r135948567
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -1073,26 +1070,22 @@ public QueryId getQueryId() {
*/
private void setupRootFragment(final PlanFragment rootFragment, final FragmentRoot rootOperator)
throws ExecutionSetupException {
- @SuppressWarnings("resource")
final FragmentContext rootContext = new FragmentContext(drillbitContext, rootFragment, queryContext,
initiatingClient, drillbitContext.getFunctionImplementationRegistry());
- @SuppressWarnings("resource")
- final IncomingBuffers buffers = new IncomingBuffers(rootFragment, rootContext);
- rootContext.setBuffers(buffers);
-
- queryManager.addFragmentStatusTracker(rootFragment, true);
-
final ControlTunnel tunnel = drillbitContext.getController().getTunnel(queryContext.getCurrentEndpoint());
+ final FragmentStatusReporter statusReporter = new FragmentStatusReporter(rootContext, tunnel);
final FragmentExecutor rootRunner = new FragmentExecutor(rootContext, rootFragment,
- new FragmentStatusReporter(rootContext, tunnel),
- rootOperator);
- final RootFragmentManager fragmentManager = new RootFragmentManager(rootFragment.getHandle(), buffers, rootRunner);
+ statusReporter, rootOperator);
- if (buffers.isDone()) {
+ queryManager.addFragmentStatusTracker(rootFragment, true);
+
+ // FragmentManager is setting buffer for FragmentContext
+ if (rootContext.isBuffersDone()) {
--- End diff --
Inside AbstractFragmentManager constructor, since that's where the Incoming Buffers are created.
https://github.com/apache/drill/pull/919/files#diff-d7418a1fd07314f268049ae9f07b7ed3
```
+ public AbstractFragmentManager(final PlanFragment fragment, final FragmentExecutor executor, final FragmentStatusReporter statusReporter, final FragmentRoot rootOperator) {
+ this.fragmentHandle = fragment.getHandle();
+ this.fragmentContext = executor.getContext();
+ this.buffers = new IncomingBuffers(fragment, fragmentContext);
+ this.fragmentContext.setBuffers(buffers);
```
---
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.
---
[GitHub] drill issue #919: DRILL-5721: Query with only root fragment and no non-root ...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/919
@parthchandra - thanks for the review!
---
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.
---
[GitHub] drill issue #919: DRILL-5721: Query with only root fragment and no non-root ...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/919
Tried to commit this one to master. The functional test run failed with 20+ "random failures." That is too many to comfortably allow. So, @sohami, please rerun this, determine if the 20+ failures are, indeed, random or due to this change, and post a note here when the PR is ready for another attempt at commit into master.
---
[GitHub] drill issue #919: DRILL-5721: Query with only root fragment and no non-root ...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/919
I rebased this PR to latest master and [re-ran functional test](http://10.10.104.91:8080/job/Functional/2447/console) and seeing only 2 random failures now. As per @agirish recommendation I have created JIRA's to track these random issues. [DRILL-5788](https://issues.apache.org/jira/browse/DRILL-5788) & [DRILL-5789](https://issues.apache.org/jira/browse/DRILL-5789).
---
[GitHub] drill issue #919: Drill-5721: Query with only root fragment and no non-root ...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/919
@parthchandra - Please help to review this PR.
---
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.
---
[GitHub] drill issue #919: DRILL-5721: Query with only root fragment and no non-root ...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/919
@paul-rogers - Last week @amansinha100 also reported 5 Random failures happening with this commit. But when ran in isolation for multiple iterations those tests were passing all the time. Can you please share the list of tests failing for you with this commit ? I can try to run a sample of those tests in isolation and see if those are really random.
---
[GitHub] drill pull request #919: DRILL-5721: Query with only root fragment and no no...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/919#discussion_r135937508
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -1109,7 +1102,7 @@ private void setupNonRootFragments(final Collection<PlanFragment> fragments) thr
// nothing to do here
return;
}
- /*
+ /*z
--- End diff --
extra character
---
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.
---
[GitHub] drill issue #919: DRILL-5721: Query with only root fragment and no non-root ...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/919
@sohami, can you clean up the stray commits and ask @parthchandra to again review the later commits?
---
[GitHub] drill issue #919: DRILL-5721: Query with only root fragment and no non-root ...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/919
OK, thanks. Got confused by the commit messages that appeared after the +1. Maybe in the future, when we rebase, just add a comment saying that's what happened.
---
[GitHub] drill pull request #919: DRILL-5721: Query with only root fragment and no no...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/919#discussion_r136161134
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java ---
@@ -1073,26 +1070,22 @@ public QueryId getQueryId() {
*/
private void setupRootFragment(final PlanFragment rootFragment, final FragmentRoot rootOperator)
throws ExecutionSetupException {
- @SuppressWarnings("resource")
final FragmentContext rootContext = new FragmentContext(drillbitContext, rootFragment, queryContext,
initiatingClient, drillbitContext.getFunctionImplementationRegistry());
- @SuppressWarnings("resource")
- final IncomingBuffers buffers = new IncomingBuffers(rootFragment, rootContext);
- rootContext.setBuffers(buffers);
-
- queryManager.addFragmentStatusTracker(rootFragment, true);
-
final ControlTunnel tunnel = drillbitContext.getController().getTunnel(queryContext.getCurrentEndpoint());
+ final FragmentStatusReporter statusReporter = new FragmentStatusReporter(rootContext, tunnel);
final FragmentExecutor rootRunner = new FragmentExecutor(rootContext, rootFragment,
- new FragmentStatusReporter(rootContext, tunnel),
- rootOperator);
- final RootFragmentManager fragmentManager = new RootFragmentManager(rootFragment.getHandle(), buffers, rootRunner);
+ statusReporter, rootOperator);
- if (buffers.isDone()) {
+ queryManager.addFragmentStatusTracker(rootFragment, true);
+
+ // FragmentManager is setting buffer for FragmentContext
+ if (rootContext.isBuffersDone()) {
--- End diff --
Yes I see it now. Sorry I missed it.
---
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.
---