Skip to content

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Nov 27, 2017

What changes were proposed in this pull request?

Submitted this PR to see if there is anything broken.

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

Please review http://spark.apache.org/contributing.html before opening a pull request.

@SparkQA
Copy link

SparkQA commented Nov 27, 2017

Test build #84226 has finished for PR 19829 at commit 14dbc71.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Nov 28, 2017

Test build #84231 has finished for PR 19829 at commit 93632a0.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@ash211
Copy link
Contributor

ash211 commented Nov 28, 2017

Looks like a fix for https://issues.apache.org/jira/browse/SPARK-19552 -- should that be reopened now that netty is deprecating 4.0.x so we can't do it "Later" anymore?

@zsxwing
Copy link
Member Author

zsxwing commented Nov 28, 2017

@ash211 Good call. I will reopen it.

return transferred;
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prototyped this change internally for a different reason, and I chose to create an abstract class for all this boilerplate, which seems cleaner to me.

public abstract class AbstractFileRegion extends AbstractReferenceCounted implements FileRegion {

  @Override
  @SuppressWarnings("deprecation")
  public final long transfered() {
    return transferred();
  }

  @Override
  public FileRegion retain() {
    super.retain();
    return this;
  }

  @Override
  public FileRegion retain(int increment) {
    super.retain(increment);
    return this;
  }

  @Override
  public FileRegion touch() {
    return touch(null);
  }

  @Override
  public FileRegion touch(Object hint) {
    return this;
  }

}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Updated!

@SparkQA
Copy link

SparkQA commented Dec 9, 2017

Test build #84668 has finished for PR 19829 at commit 94df9ae.

  • This patch fails build dependency tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class MessageWithHeader extends AbstractFileRegion
  • static class EncryptedMessage extends AbstractFileRegion
  • public abstract class AbstractFileRegion extends AbstractReferenceCounted implements FileRegion

@SparkQA
Copy link

SparkQA commented Dec 9, 2017

Test build #84669 has finished for PR 19829 at commit 9a07d2b.

  • This patch fails build dependency tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Dec 9, 2017

Test build #84670 has finished for PR 19829 at commit c7cffe9.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class MessageWithHeader extends AbstractFileRegion
  • static class EncryptedMessage extends AbstractFileRegion
  • public abstract class AbstractFileRegion extends AbstractReferenceCounted implements FileRegion

@SparkQA
Copy link

SparkQA commented Dec 9, 2017

Test build #84671 has finished for PR 19829 at commit 96df5f2.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class MessageWithHeader extends AbstractFileRegion
  • static class EncryptedMessage extends AbstractFileRegion
  • public abstract class AbstractFileRegion extends AbstractReferenceCounted implements FileRegion

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants