New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRILL-6153: Operator framework #1121

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@paul-rogers
Contributor

paul-rogers commented Feb 13, 2018

Includes the core files for the operator framework revision. See this writeup for details.

In this commit, nothing depends on this new code. It is, instead, the foundation for the revised scan operator to be added after the revised result set loader code is committed. Doing this commit now allows small PRs to be done in parallel.

@ilooner

This comment has been minimized.

Show comment
Hide comment
@ilooner

ilooner Feb 13, 2018

Contributor

Hi @paul-rogers, nice to see this code being encapsulated, standardized, and unit tested. There seems to be a Travis test failure with your changes though.

Failed tests: 
  TestOperatorRecordBatch.testNormalLifeCycle:156 expected null, but was:<org.apache.drill.test.OperatorFixture$MockFragmentContext@45f1feca>
Contributor

ilooner commented Feb 13, 2018

Hi @paul-rogers, nice to see this code being encapsulated, standardized, and unit tested. There seems to be a Travis test failure with your changes though.

Failed tests: 
  TestOperatorRecordBatch.testNormalLifeCycle:156 expected null, but was:<org.apache.drill.test.OperatorFixture$MockFragmentContext@45f1feca>
Show outdated Hide outdated ...in/java/org/apache/drill/exec/physical/impl/protocol/OperatorDriver.java
}
public void close() {
if (state == State.CLOSED) {

This comment has been minimized.

@ppadma

ppadma Feb 14, 2018

Contributor

should we check for FAILED and END state also here ?

@ppadma

ppadma Feb 14, 2018

Contributor

should we check for FAILED and END state also here ?

This comment has been minimized.

@paul-rogers

paul-rogers Feb 16, 2018

Contributor

Turns out the check is not needed. The goal here is simply to silently catch duplicate calls to close(). Whether the operator succeeded or failed, the implementation still must release its resources. But, by catching repeated close() calls, the implementation can assume that its own close() is called exactly once.

Note that END/CANCELED/FAILED is a separate state from CLOSED. The former says no more batches will be returned, but close() has not yet been called. The later says that close() has been called and the show is over.

@paul-rogers

paul-rogers Feb 16, 2018

Contributor

Turns out the check is not needed. The goal here is simply to silently catch duplicate calls to close(). Whether the operator succeeded or failed, the implementation still must release its resources. But, by catching repeated close() calls, the implementation can assume that its own close() is called exactly once.

Note that END/CANCELED/FAILED is a separate state from CLOSED. The former says no more batches will be returned, but close() has not yet been called. The later says that close() has been called and the show is over.

/**
* Alerts the operator that the query was cancelled. Generally
* optional, but allows the operator to realize that a cancellation

This comment has been minimized.

@ppadma

ppadma Feb 14, 2018

Contributor

why is this optional ?

@ppadma

ppadma Feb 14, 2018

Contributor

why is this optional ?

This comment has been minimized.

@paul-rogers

paul-rogers Feb 16, 2018

Contributor

An operator will work if it ignores cancel(). It's close() method should "do the right thing":

  • If the operator reached EOF
  • If the operator did not reach EOF (still rows left)
  • If the operator failed

The cancel() call is just a hint that "I won't be reading any more rows; feel free to release resources now if you like."

@paul-rogers

paul-rogers Feb 16, 2018

Contributor

An operator will work if it ignores cancel(). It's close() method should "do the right thing":

  • If the operator reached EOF
  • If the operator did not reach EOF (still rows left)
  • If the operator failed

The cancel() call is just a hint that "I won't be reading any more rows; feel free to release resources now if you like."

Show outdated Hide outdated ...va/org/apache/drill/exec/physical/impl/protocol/OperatorRecordBatch.java
Show outdated Hide outdated ...in/java/org/apache/drill/exec/physical/impl/protocol/OperatorDriver.java
* only a schema, or EOF
*/
private IterOutcome start() {

This comment has been minimized.

@ppadma

ppadma Feb 14, 2018

Contributor

would be good if we can capture what exceptions each of these functions might throw.

@ppadma

ppadma Feb 14, 2018

Contributor

would be good if we can capture what exceptions each of these functions might throw.

This comment has been minimized.

@paul-rogers

paul-rogers Feb 16, 2018

Contributor

Somewhere I explained the policy assumed here:

  • Implementations of the OperatorExec interface are responsible for error handling.
  • Implementations should catch exceptions then translate them to a UserException with user-oriented information.
  • If implementations do the above, this layer simply passes along the UserException.
  • Otherwise, this class acts as a translation layer to convert generic unchecked exceptions into UserExceptions. This layer can't provide much context; but it can do a slightly better job than the fragment exec, which is the last line of defense.

As a result, this layer always throws UserExceptions.

@paul-rogers

paul-rogers Feb 16, 2018

Contributor

Somewhere I explained the policy assumed here:

  • Implementations of the OperatorExec interface are responsible for error handling.
  • Implementations should catch exceptions then translate them to a UserException with user-oriented information.
  • If implementations do the above, this layer simply passes along the UserException.
  • Otherwise, this class acts as a translation layer to convert generic unchecked exceptions into UserExceptions. This layer can't provide much context; but it can do a slightly better job than the fragment exec, which is the last line of defense.

As a result, this layer always throws UserExceptions.

@paul-rogers

@ppadma, thanks for the review. Fixes and comments made.

Show outdated Hide outdated ...in/java/org/apache/drill/exec/physical/impl/protocol/OperatorDriver.java
Show outdated Hide outdated ...in/java/org/apache/drill/exec/physical/impl/protocol/OperatorDriver.java
Show outdated Hide outdated ...in/java/org/apache/drill/exec/physical/impl/protocol/OperatorDriver.java
* only a schema, or EOF
*/
private IterOutcome start() {

This comment has been minimized.

@paul-rogers

paul-rogers Feb 16, 2018

Contributor

Somewhere I explained the policy assumed here:

  • Implementations of the OperatorExec interface are responsible for error handling.
  • Implementations should catch exceptions then translate them to a UserException with user-oriented information.
  • If implementations do the above, this layer simply passes along the UserException.
  • Otherwise, this class acts as a translation layer to convert generic unchecked exceptions into UserExceptions. This layer can't provide much context; but it can do a slightly better job than the fragment exec, which is the last line of defense.

As a result, this layer always throws UserExceptions.

@paul-rogers

paul-rogers Feb 16, 2018

Contributor

Somewhere I explained the policy assumed here:

  • Implementations of the OperatorExec interface are responsible for error handling.
  • Implementations should catch exceptions then translate them to a UserException with user-oriented information.
  • If implementations do the above, this layer simply passes along the UserException.
  • Otherwise, this class acts as a translation layer to convert generic unchecked exceptions into UserExceptions. This layer can't provide much context; but it can do a slightly better job than the fragment exec, which is the last line of defense.

As a result, this layer always throws UserExceptions.

}
public void close() {
if (state == State.CLOSED) {

This comment has been minimized.

@paul-rogers

paul-rogers Feb 16, 2018

Contributor

Turns out the check is not needed. The goal here is simply to silently catch duplicate calls to close(). Whether the operator succeeded or failed, the implementation still must release its resources. But, by catching repeated close() calls, the implementation can assume that its own close() is called exactly once.

Note that END/CANCELED/FAILED is a separate state from CLOSED. The former says no more batches will be returned, but close() has not yet been called. The later says that close() has been called and the show is over.

@paul-rogers

paul-rogers Feb 16, 2018

Contributor

Turns out the check is not needed. The goal here is simply to silently catch duplicate calls to close(). Whether the operator succeeded or failed, the implementation still must release its resources. But, by catching repeated close() calls, the implementation can assume that its own close() is called exactly once.

Note that END/CANCELED/FAILED is a separate state from CLOSED. The former says no more batches will be returned, but close() has not yet been called. The later says that close() has been called and the show is over.

/**
* Alerts the operator that the query was cancelled. Generally
* optional, but allows the operator to realize that a cancellation

This comment has been minimized.

@paul-rogers

paul-rogers Feb 16, 2018

Contributor

An operator will work if it ignores cancel(). It's close() method should "do the right thing":

  • If the operator reached EOF
  • If the operator did not reach EOF (still rows left)
  • If the operator failed

The cancel() call is just a hint that "I won't be reading any more rows; feel free to release resources now if you like."

@paul-rogers

paul-rogers Feb 16, 2018

Contributor

An operator will work if it ignores cancel(). It's close() method should "do the right thing":

  • If the operator reached EOF
  • If the operator did not reach EOF (still rows left)
  • If the operator failed

The cancel() call is just a hint that "I won't be reading any more rows; feel free to release resources now if you like."

@paul-rogers

This comment has been minimized.

Show comment
Hide comment
@paul-rogers

paul-rogers Feb 20, 2018

Contributor

Rebased on latest master.

Contributor

paul-rogers commented Feb 20, 2018

Rebased on latest master.

@paul-rogers

This comment has been minimized.

Show comment
Hide comment
@paul-rogers

paul-rogers Feb 27, 2018

Contributor

@arina-ielchiieva, can you do a committer review of this one?

Contributor

paul-rogers commented Feb 27, 2018

@arina-ielchiieva, can you do a committer review of this one?

arina-ielchiieva added a commit to arina-ielchiieva/drill that referenced this pull request Mar 3, 2018

@asfgit asfgit closed this in 69a5f3a Mar 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment