You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by kkhatua <gi...@git.apache.org> on 2017/11/29 19:44:17 UTC
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
GitHub user kkhatua opened a pull request:
https://github.com/apache/drill/pull/1055
DRILL-5973 : Support injection of time-bound pauses in server
Support pause injections in the test framework that are time-bound, to allow for testing high latency scenarios.
e.g. delayed server response to the Drill client allows for test a server-induced timeout
This would allow for testing of DRILL-3640
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kkhatua/drill DRILL-5973
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1055.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 #1055
----
commit 62e6f721183d648797d5329e94b277cd5722bba6
Author: Kunal Khatua <kk...@maprtech.com>
Date: 2017-11-29T19:00:11Z
DRILL-5973 : Support injection of time-bound pauses in server
Support pause injections in the test framework that are time-bound, to allow for testing high latency scenarios.
e.g. delayed server response to the Drill client allows for test a server-induced timeout
----
---
[GitHub] drill issue #1055: DRILL-5973 : Support injection of time-bound pauses in se...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/1055
@parthchandra , the code was already rebased. The failing unit tests were related to TestCountDownLatchInjection, which had APIs that were unnecessarily extended to incorporate pause durations. Reverting it back to the original constructor resolved the issues and both, previously failing unit tests passed. Waiting for the rest of the regressions to pass.
---
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/1055#discussion_r154247977
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java ---
@@ -144,6 +144,34 @@ public void pauseInjected() {
}
}
+ @Test
+ public void timedPauseInjected() {
+ final long pauseDuration = 2000L;
+ final long expectedDuration = pauseDuration;
+ final ExtendedLatch trigger = new ExtendedLatch(1);
+ final Pointer<Exception> ex = new Pointer<>();
+ final String controls = Controls.newBuilder()
+ .addTimedPause(DummyClass.class, DummyClass.PAUSES, 0, pauseDuration)
+ .build();
+
+ ControlsInjectionUtil.setControls(session, controls);
+
+ final QueryContext queryContext = new QueryContext(session, bits[0].getContext(), QueryId.getDefaultInstance());
+ //We don't need a ResumingThread because this should automatically resume
+
+ // test that the pause happens
+ final DummyClass dummyClass = new DummyClass(queryContext, trigger);
+ final long actualDuration = dummyClass.pauses();
+ assertTrue(String.format("Test should stop for at least %d milliseconds.", expectedDuration),
+ expectedDuration <= actualDuration);
+ assertTrue("No exception should be thrown.", ex.value == null);
+ try {
+ queryContext.close();
+ } catch (final Exception e) {
+ fail("Failed to close query context: " + e);
--- End diff --
maybe better let JUnit handle unexpected expection (at least you would get the whole stack trace instead of just the error message) (I guess it's not new code, so feel free to ignore)
---
[GitHub] drill issue #1055: DRILL-5973 : Support injection of time-bound pauses in se...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/1055
Sorry. Pushing in changes and squashing simultaneously. Made the relevant changes, except for the check
```
long pauseDuration = pauseInjection.getMsPause();
if ( pauseDuration > 0L) {
```
---
[GitHub] drill issue #1055: DRILL-5973 : Support injection of time-bound pauses in se...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/1055
@parthchandra Passed all regression and unit tests. Travis build is failing due to the GraceFullShutdown tests. Let me know if I should squash and rebase.
---
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/1055#discussion_r154150476
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java ---
@@ -81,8 +83,20 @@ public void injectPause(final ExecutionControls executionControls, final String
executionControls.lookupPauseInjection(this, desc);
if (pauseInjection != null) {
- logger.debug("Pausing at {}", desc);
- pauseInjection.pause();
+ long pauseDuration = pauseInjection.getMsPause();
+ if ( pauseDuration > 0L) {
--- End diff --
maybe nitpicky but I would use -1 (or any negative value) to distinguish between timed-bounded pause or not (since 0 is a valid wait time for CountDownLatch)
---
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/1055#discussion_r154248201
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java ---
@@ -81,8 +83,20 @@ public void injectPause(final ExecutionControls executionControls, final String
executionControls.lookupPauseInjection(this, desc);
if (pauseInjection != null) {
- logger.debug("Pausing at {}", desc);
- pauseInjection.pause();
+ long pauseDuration = pauseInjection.getMsPause();
+ if ( pauseDuration > 0L) {
+ logger.debug("Pausing at {} for {} sec", desc, TimeUnit.MILLISECONDS.toSeconds(pauseDuration) );
+ } else {
+ logger.debug("Pausing at {}", desc);
+ }
+ try {
+ pauseInjection.pause();
+ } catch (InterruptedException e) {
+ //Unpause if this is a timed pause, because an explicit unpause is not called
+ if (pauseDuration > 0L) {
--- End diff --
did you forget to remove that part following up on your other change?
---
[GitHub] drill issue #1055: DRILL-5973 : Support injection of time-bound pauses in se...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on the issue:
https://github.com/apache/drill/pull/1055
This PR seems to be failing a bunch of unit that don't seem related. do you need to rebase ?
---
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on a diff in the pull request:
https://github.com/apache/drill/pull/1055#discussion_r154248904
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/testing/TestPauseInjection.java ---
@@ -144,6 +144,34 @@ public void pauseInjected() {
}
}
+ @Test
+ public void timedPauseInjected() {
+ final long pauseDuration = 2000L;
+ final long expectedDuration = pauseDuration;
+ final ExtendedLatch trigger = new ExtendedLatch(1);
+ final Pointer<Exception> ex = new Pointer<>();
+ final String controls = Controls.newBuilder()
+ .addTimedPause(DummyClass.class, DummyClass.PAUSES, 0, pauseDuration)
+ .build();
+
+ ControlsInjectionUtil.setControls(session, controls);
+
+ final QueryContext queryContext = new QueryContext(session, bits[0].getContext(), QueryId.getDefaultInstance());
+ //We don't need a ResumingThread because this should automatically resume
+
+ // test that the pause happens
+ final DummyClass dummyClass = new DummyClass(queryContext, trigger);
+ final long actualDuration = dummyClass.pauses();
+ assertTrue(String.format("Test should stop for at least %d milliseconds.", expectedDuration),
+ expectedDuration <= actualDuration);
+ assertTrue("No exception should be thrown.", ex.value == null);
+ try {
+ queryContext.close();
+ } catch (final Exception e) {
+ fail("Failed to close query context: " + e);
--- End diff --
```
did you forget to remove that part following up on your other change?
```
Didn't forget to remove. It drives the logger to publish the message in the debugger as time-bound if it is non-zero.
---
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on a diff in the pull request:
https://github.com/apache/drill/pull/1055#discussion_r155877298
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java ---
@@ -41,8 +41,11 @@
private CountDownLatchInjectionImpl(@JsonProperty("address") final String address,
@JsonProperty("port") final int port,
@JsonProperty("siteClass") final String siteClass,
- @JsonProperty("desc") final String desc) throws InjectionConfigurationException {
- super(address, port, siteClass, desc, 0, 1);
+ @JsonProperty("desc") final String desc,
+ @JsonProperty("nSkip") final int nSkip,
+ @JsonProperty("nFire") final int nFire,
+ @JsonProperty("msPause") final Long msPause) throws InjectionConfigurationException {
--- End diff --
Might have hit it accidentally. Don't think I need a null check. Will walk through this once and update it.
---
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on a diff in the pull request:
https://github.com/apache/drill/pull/1055#discussion_r154247418
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java ---
@@ -81,8 +83,20 @@ public void injectPause(final ExecutionControls executionControls, final String
executionControls.lookupPauseInjection(this, desc);
if (pauseInjection != null) {
- logger.debug("Pausing at {}", desc);
- pauseInjection.pause();
+ long pauseDuration = pauseInjection.getMsPause();
+ if ( pauseDuration > 0L) {
+ logger.debug("Pausing at {} for {} sec", desc, TimeUnit.MILLISECONDS.toSeconds(pauseDuration) );
+ } else {
+ logger.debug("Pausing at {}", desc);
+ }
+ try {
+ pauseInjection.pause();
+ } catch (InterruptedException e) {
+ //Unpause if this is a timed pause, because an explicit unpause is not called
+ if (pauseDuration > 0L) {
--- End diff --
Will make the change since the InterruptedException is being handled in the timed pause (based on your comment below).
---
[GitHub] drill issue #1055: DRILL-5973 : Support injection of time-bound pauses in se...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/1055
Shouldn't be failing. I think I'll rebase and run pre-commit tests to confirm. Thanks!
---
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/1055#discussion_r154153787
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java ---
@@ -81,8 +83,20 @@ public void injectPause(final ExecutionControls executionControls, final String
executionControls.lookupPauseInjection(this, desc);
if (pauseInjection != null) {
- logger.debug("Pausing at {}", desc);
- pauseInjection.pause();
+ long pauseDuration = pauseInjection.getMsPause();
+ if ( pauseDuration > 0L) {
+ logger.debug("Pausing at {} for {} sec", desc, TimeUnit.MILLISECONDS.toSeconds(pauseDuration) );
+ } else {
+ logger.debug("Pausing at {}", desc);
+ }
+ try {
+ pauseInjection.pause();
+ } catch (InterruptedException e) {
+ //Unpause if this is a timed pause, because an explicit unpause is not called
+ if (pauseDuration > 0L) {
--- End diff --
maybe to be done in PauseInjection (seems like something internal)
---
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on a diff in the pull request:
https://github.com/apache/drill/pull/1055#discussion_r154246413
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java ---
@@ -81,8 +83,20 @@ public void injectPause(final ExecutionControls executionControls, final String
executionControls.lookupPauseInjection(this, desc);
if (pauseInjection != null) {
- logger.debug("Pausing at {}", desc);
- pauseInjection.pause();
+ long pauseDuration = pauseInjection.getMsPause();
+ if ( pauseDuration > 0L) {
--- End diff --
Agreed, but the only purpose of that check is to indicate during debugging that a non-zero timed pause was injected. Hence, the check.
---
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1055#discussion_r155326676
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/testing/CountDownLatchInjectionImpl.java ---
@@ -41,8 +41,11 @@
private CountDownLatchInjectionImpl(@JsonProperty("address") final String address,
@JsonProperty("port") final int port,
@JsonProperty("siteClass") final String siteClass,
- @JsonProperty("desc") final String desc) throws InjectionConfigurationException {
- super(address, port, siteClass, desc, 0, 1);
+ @JsonProperty("desc") final String desc,
+ @JsonProperty("nSkip") final int nSkip,
+ @JsonProperty("nFire") final int nFire,
+ @JsonProperty("msPause") final Long msPause) throws InjectionConfigurationException {
--- End diff --
long instead of Long? You will save a lot of unnecessary checking for null
---
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1055
---
[GitHub] drill issue #1055: DRILL-5973 : Support injection of time-bound pauses in se...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/1055
Didn't make sense to have small commits in the PR. So I've squashed and rebased onto 1.13.0.
---
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on a diff in the pull request:
https://github.com/apache/drill/pull/1055#discussion_r154246723
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/testing/ExecutionControlsInjector.java ---
@@ -81,8 +83,20 @@ public void injectPause(final ExecutionControls executionControls, final String
executionControls.lookupPauseInjection(this, desc);
if (pauseInjection != null) {
- logger.debug("Pausing at {}", desc);
- pauseInjection.pause();
+ long pauseDuration = pauseInjection.getMsPause();
+ if ( pauseDuration > 0L) {
+ logger.debug("Pausing at {} for {} sec", desc, TimeUnit.MILLISECONDS.toSeconds(pauseDuration) );
+ } else {
+ logger.debug("Pausing at {}", desc);
+ }
+ try {
+ pauseInjection.pause();
+ } catch (InterruptedException e) {
+ //Unpause if this is a timed pause, because an explicit unpause is not called
+ if (pauseDuration > 0L) {
--- End diff --
Just following convention of 'unpausing' explicitly, rather than implicitly within the `pause()` for a limited-duration pause.
---
[GitHub] drill issue #1055: DRILL-5973 : Support injection of time-bound pauses in se...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/1055
Verified the PauseInjection tests and that the commit builds and works normally.
---
[GitHub] drill issue #1055: DRILL-5973 : Support injection of time-bound pauses in se...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on the issue:
https://github.com/apache/drill/pull/1055
+1.
No need to squash/rebase.
---
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/1055#discussion_r154153939
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java ---
@@ -44,14 +46,31 @@ private PauseInjection(@JsonProperty("address") final String address,
@JsonProperty("siteClass") final String siteClass,
@JsonProperty("desc") final String desc,
@JsonProperty("nSkip") final int nSkip) throws InjectionConfigurationException {
- super(address, port, siteClass, desc, nSkip, 1);
+ //nFire is 1 since we will inject pauses only once
+ //msPause is 0 (i.e. not time-bound)
+ super(address, port, siteClass, desc, nSkip, 1, 0L);
+ }
+
+ @JsonCreator // ensures instances are created only through JSON
+ private PauseInjection(@JsonProperty("address") final String address,
+ @JsonProperty("port") final int port,
+ @JsonProperty("siteClass") final String siteClass,
+ @JsonProperty("desc") final String desc,
+ @JsonProperty("nSkip") final int nSkip,
+ @JsonProperty("msPause") final long msPause) throws InjectionConfigurationException {
+ //nFire is 1 since we will inject pauses only once
+ super(address, port, siteClass, desc, nSkip, 1, msPause);
}
- public void pause() {
+ public void pause() throws InterruptedException {
--- End diff --
I would not expose the interruption but handle it internally instead (by decreasing the latch counter)
---
[GitHub] drill issue #1055: DRILL-5973 : Support injection of time-bound pauses in se...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/1055
@laurentgo / @parthchandra
Please review this. It is the basis for unit tests in DRILL-3640
---
[GitHub] drill pull request #1055: DRILL-5973 : Support injection of time-bound pause...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on a diff in the pull request:
https://github.com/apache/drill/pull/1055#discussion_r154247369
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/testing/PauseInjection.java ---
@@ -44,14 +46,31 @@ private PauseInjection(@JsonProperty("address") final String address,
@JsonProperty("siteClass") final String siteClass,
@JsonProperty("desc") final String desc,
@JsonProperty("nSkip") final int nSkip) throws InjectionConfigurationException {
- super(address, port, siteClass, desc, nSkip, 1);
+ //nFire is 1 since we will inject pauses only once
+ //msPause is 0 (i.e. not time-bound)
+ super(address, port, siteClass, desc, nSkip, 1, 0L);
+ }
+
+ @JsonCreator // ensures instances are created only through JSON
+ private PauseInjection(@JsonProperty("address") final String address,
+ @JsonProperty("port") final int port,
+ @JsonProperty("siteClass") final String siteClass,
+ @JsonProperty("desc") final String desc,
+ @JsonProperty("nSkip") final int nSkip,
+ @JsonProperty("msPause") final long msPause) throws InjectionConfigurationException {
+ //nFire is 1 since we will inject pauses only once
+ super(address, port, siteClass, desc, nSkip, 1, msPause);
}
- public void pause() {
+ public void pause() throws InterruptedException {
--- End diff --
Agreed.
---