Skip to content

Conversation

@HeartSaVioR
Copy link
Contributor

@HeartSaVioR HeartSaVioR commented Feb 14, 2020

What changes were proposed in this pull request?

This patch adds direct relationship among configurations under "spark.history" namespace.

Why are the changes needed?

Refer the discussion thread: https://lists.apache.org/thread.html/r43c4e57cace116aca1f0f099e8a577cf202859e3671a04077867b84a%40%3Cdev.spark.apache.org%3E

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Locally ran jekyll and confirmed. Screenshots for the modified spots:

Screen Shot 2020-02-15 at 8 20 14 PM

Screen Shot 2020-02-15 at 8 20 44 PM

Screen Shot 2020-02-15 at 8 19 56 PM

@HeartSaVioR
Copy link
Contributor Author

cc. @HyukjinKwon

@SparkQA
Copy link

SparkQA commented Feb 14, 2020

Test build #118398 has finished for PR 27575 at commit 23aef36.

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

@HeartSaVioR
Copy link
Contributor Author

retest this, please

<td>spark.history.kerberos.principal</td>
<td>(none)</td>
<td>
Precondition: <code>spark.history.kerberos.enabled=true</code><br/><br/>
Copy link
Member

Choose a reason for hiding this comment

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

@HeartSaVioR, seems Precondition: is new style. Can we follow one prevailing style we use in the current code base? Or I actually think it's just better to say "it's effective when spark.xx.xx is enabled." at the end of each description. Current one seems catching too much attention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm seeing different styles in current codebase - I've sought three cases and styles are all different:

spark.dynamicAllocation

if dynamic allocation is enabled (either placed on the first or the last)

spark.task.reaper.*

When spark.task.reaper.enabled = true

spark.sql.adaptive.shuffle.reducePostShufflePartitions.enabled

When true and '${ADAPTIVE_EXECUTION_ENABLED.key}' is enabled,

Common part across these finding is that we prefer putting it to the first part so that end user can indicate it earlier instead of reading all description and realized it later.

Looks like second and third style look similar, I'll take these style and update here.

@SparkQA
Copy link

SparkQA commented Feb 14, 2020

Test build #118406 has finished for PR 27575 at commit 23aef36.

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

@HeartSaVioR
Copy link
Contributor Author

Update the doc as well as PR description. Thanks!

@SparkQA
Copy link

SparkQA commented Feb 15, 2020

Test build #118475 has finished for PR 27575 at commit f187060.

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

@HeartSaVioR
Copy link
Contributor Author

retest this, please

@SparkQA
Copy link

SparkQA commented Feb 15, 2020

Test build #118478 has finished for PR 27575 at commit f187060.

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

@HyukjinKwon
Copy link
Member

I am merging it. @HeartSaVioR, do you plan to go through all documentations for such instances? If you plan to do so, I think it's just better to go and fix as we discussed in the mailing list:

  • Check and fix all such configurations to be structured
  • Just document somewhere what the structure implies

HyukjinKwon pushed a commit that referenced this pull request Feb 17, 2020
… in "spark.history.*" namespace

### What changes were proposed in this pull request?

This patch adds direct relationship among configurations under "spark.history" namespace.

### Why are the changes needed?

Refer the discussion thread: https://lists.apache.org/thread.html/r43c4e57cace116aca1f0f099e8a577cf202859e3671a04077867b84a%40%3Cdev.spark.apache.org%3E

### Does this PR introduce any user-facing change?

No.

### How was this patch tested?

Locally ran jekyll and confirmed. Screenshots for the modified spots:

<img width="1159" alt="Screen Shot 2020-02-15 at 8 20 14 PM" src="https://user-images.githubusercontent.com/1317309/74587003-d5922b00-5030-11ea-954b-ee37fc08470a.png">
<img width="1158" alt="Screen Shot 2020-02-15 at 8 20 44 PM" src="https://user-images.githubusercontent.com/1317309/74587005-d62ac180-5030-11ea-98fc-98b1c9d83ff4.png">
<img width="1149" alt="Screen Shot 2020-02-15 at 8 19 56 PM" src="https://user-images.githubusercontent.com/1317309/74587002-d1660d80-5030-11ea-84b5-dec3d7f5c97c.png">

Closes #27575 from HeartSaVioR/SPARK-30827.

Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
(cherry picked from commit 5445fe9)
Signed-off-by: HyukjinKwon <[email protected]>
@HyukjinKwon
Copy link
Member

Merged to master and branch-3.0.

@HeartSaVioR
Copy link
Contributor Author

Thanks for reviewing and merging!

@HeartSaVioR, do you plan to go through all documentations for such instances?

No. I just fixed them as we are aware of them. Definitely better to fix the structure instead.

@HeartSaVioR HeartSaVioR deleted the SPARK-30827 branch February 17, 2020 12:21
sjincho pushed a commit to sjincho/spark that referenced this pull request Apr 15, 2020
… in "spark.history.*" namespace

### What changes were proposed in this pull request?

This patch adds direct relationship among configurations under "spark.history" namespace.

### Why are the changes needed?

Refer the discussion thread: https://lists.apache.org/thread.html/r43c4e57cace116aca1f0f099e8a577cf202859e3671a04077867b84a%40%3Cdev.spark.apache.org%3E

### Does this PR introduce any user-facing change?

No.

### How was this patch tested?

Locally ran jekyll and confirmed. Screenshots for the modified spots:

<img width="1159" alt="Screen Shot 2020-02-15 at 8 20 14 PM" src="https://user-images.githubusercontent.com/1317309/74587003-d5922b00-5030-11ea-954b-ee37fc08470a.png">
<img width="1158" alt="Screen Shot 2020-02-15 at 8 20 44 PM" src="https://user-images.githubusercontent.com/1317309/74587005-d62ac180-5030-11ea-98fc-98b1c9d83ff4.png">
<img width="1149" alt="Screen Shot 2020-02-15 at 8 19 56 PM" src="https://user-images.githubusercontent.com/1317309/74587002-d1660d80-5030-11ea-84b5-dec3d7f5c97c.png">

Closes apache#27575 from HeartSaVioR/SPARK-30827.

Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
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.

3 participants