Skip to content

[SPARK-27215][CORE] Correct the kryo configurations - #24156

Closed
LantaoJin wants to merge 1 commit into
apache:masterfrom
LantaoJin:SPARK-27215
Closed

[SPARK-27215][CORE] Correct the kryo configurations#24156
LantaoJin wants to merge 1 commit into
apache:masterfrom
LantaoJin:SPARK-27215

Conversation

@LantaoJin

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

valKRYO_USE_UNSAFE=ConfigBuilder("spark.kyro.unsafe")
.booleanConf
.createWithDefault(false)
valKRYO_USE_POOL=ConfigBuilder("spark.kyro.pool")
.booleanConf
.createWithDefault(true)

kyro should be kryo

How was this patch tested?

no need

@LantaoJin

Copy link
Copy Markdown
ContributorAuthor

@vanzin@squito@gatorsmile, could you have a time to review this?

@attilapirosattilapiros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

I have checked the typo was introduced by #23532 which went into 3.0.0-SNAPSHOT and anyway the whole Kryo pool (along with this config) was introduced only in 3.0.0-SNAPSHOT.

Moreover there is no more reference to the incorrect spelling:

attilapiros@apiros-MBP ~/github/spark (pr/24156) $ find . \( -name "*.scala" -or -name "*.java" \) -exec grep -i "kyro" \{} \; -print
attilapiros@apiros-MBP ~/github/spark (pr/24156) $

@gaborgsomogyigaborgsomogyi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

@jiangxb1987

Copy link
Copy Markdown
Contributor

Does this change means the previous config key will no longer be recognized?

@vanzin

Copy link
Copy Markdown
Contributor

Does this change means the previous config key will no longer be recognized?

The "previous" config key never existed, it was a typo in a recent change.

@jiangxb1987

Copy link
Copy Markdown
Contributor

Does this change means the previous config key will no longer be recognized?

The "previous" config key never existed, it was a typo in a recent change.

Thanks, sounds good!

@dongjoon-hyun

Copy link
Copy Markdown
Member

cc @HeartSaVioR

@HeartSaVioRHeartSaVioR left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing my missed spots.

@SparkQA

Copy link
Copy Markdown

Test build #4642 has finished for PR 24156 at commit 616c8ce.

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

@vanzin

Copy link
Copy Markdown
Contributor

Merging to master.

Sign up for freeto 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.

9 participants

@LantaoJin@jiangxb1987@vanzin@dongjoon-hyun@SparkQA@srowen@HeartSaVioR@attilapiros@gaborgsomogyi