Skip to content
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

[SPARK-13652][Core]Copy ByteBuffer in sendRpcSync as it will be recycled #11499

Closed
wants to merge 1 commit into from
Closed

[SPARK-13652][Core]Copy ByteBuffer in sendRpcSync as it will be recycled #11499

wants to merge 1 commit into from

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Mar 3, 2016

What changes were proposed in this pull request?

sendRpcSync should copy the response content because the underlying buffer will be recycled and reused.

How was this patch tested?

Jenkins unit tests.

@zsxwing
Copy link
Member Author

zsxwing commented Mar 3, 2016

cc @vanzin
Also cc @yaooqinn as Sasl uses this method and I guess this may be the cause you reported in #9987 (comment)

@srowen
Copy link
Member

srowen commented Mar 3, 2016

Does this situation arise when there aren't multiple threads accessing one stream? I had thought the docs were prohibiting this so that the defensive copy wasn't needed

@zsxwing
Copy link
Member Author

zsxwing commented Mar 3, 2016

Does this situation arise when there aren't multiple threads accessing one stream? I had thought the docs were prohibiting this so that the defensive copy wasn't needed

This is not related to multiple threads accessing one stream. The caller of sendRpcSync is usually in some user thread and onSuccess is called in netty's thread pool. They are not in the same thread.

However, the doc is pretty confusing. I think it wants to say that if using different clients to send messages, the responses may be out-of-order.

@vanzin
Copy link
Contributor

vanzin commented Mar 3, 2016

LGTM.

@SparkQA
Copy link

SparkQA commented Mar 4, 2016

Test build #52411 has finished for PR 11499 at commit 7199237.

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

@zsxwing
Copy link
Member Author

zsxwing commented Mar 4, 2016

It's a known flaky test. Retest this please

@SparkQA
Copy link

SparkQA commented Mar 4, 2016

Test build #52426 has finished for PR 11499 at commit 7199237.

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

@zsxwing
Copy link
Member Author

zsxwing commented Mar 4, 2016

retest this please

@SparkQA
Copy link

SparkQA commented Mar 4, 2016

Test build #52445 has finished for PR 11499 at commit 7199237.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@zsxwing
Copy link
Member Author

zsxwing commented Mar 4, 2016

Merging to master and 1.6

asfgit pushed a commit that referenced this pull request Mar 4, 2016
…cled

## What changes were proposed in this pull request?

`sendRpcSync` should copy the response content because the underlying buffer will be recycled and reused.

## How was this patch tested?

Jenkins unit tests.

Author: Shixiong Zhu <shixiong@databricks.com>

Closes #11499 from zsxwing/SPARK-13652.

(cherry picked from commit 465c665)
Signed-off-by: Shixiong Zhu <shixiong@databricks.com>
@asfgit asfgit closed this in 465c665 Mar 4, 2016
@zsxwing zsxwing deleted the SPARK-13652 branch March 4, 2016 18:08
@zsxwing
Copy link
Member Author

zsxwing commented Mar 4, 2016

@yaooqinn could you also test this patch using your application to see if this one resolves your problem? Thanks!

@viper-kun
Copy link
Contributor

@zsxwing It is ok.

@yaooqinn
Copy link
Member

yaooqinn commented Mar 6, 2016

LGTM,Thanks!

roygao94 pushed a commit to roygao94/spark that referenced this pull request Mar 22, 2016
…cled

## What changes were proposed in this pull request?

`sendRpcSync` should copy the response content because the underlying buffer will be recycled and reused.

## How was this patch tested?

Jenkins unit tests.

Author: Shixiong Zhu <shixiong@databricks.com>

Closes apache#11499 from zsxwing/SPARK-13652.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants