Skip to content

Conversation

@gaborgsomogyi
Copy link
Contributor

What changes were proposed in this pull request?

The spark properties for configuring the ContextCleaner are not documented in the official documentation at https://spark.apache.org/docs/latest/configuration.html#available-properties.

This PR adds the doc.

How was this patch tested?

Manual.

cd docs
jekyll build
open _site/configuration.html

@SparkQA
Copy link

SparkQA commented Nov 27, 2017

Test build #3994 has finished for PR 19826 at commit cbd9d68.

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

<td>
Controls whether the cleaning thread should block on cleanup tasks (other than shuffle, which is controlled by
spark.cleaner.referenceTracking.blocking.shuffle Spark property).<br><br>
It is true as a workaround to <a href="https://issues.apache.org/jira/browse/SPARK-3015">
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove comments like this, or leave them to the source code. This doesn't directly help the reader understand when or if to change it. Or, add context here about why one would set this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

<td>true</td>
<td>
Controls whether the cleaning thread should block on cleanup tasks (other than shuffle, which is controlled by
spark.cleaner.referenceTracking.blocking.shuffle Spark property).<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.

You might surround prop names in <code> for extra clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surrounded.

<td><code>spark.cleaner.periodicGC.interval</code></td>
<td>30min</td>
<td>
Controls how often to trigger a garbage collection.
Copy link
Member

Choose a reason for hiding this comment

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

You might give at least a sentence of explanation about what this has to do with the context cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given more detailed description.

<td><code>spark.cleaner.referenceTracking</code></td>
<td>true</td>
<td>
Controls whether a ContextCleaner should be created when a SparkContext initializes.
Copy link
Member

Choose a reason for hiding this comment

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

I'd just say it controls whether context cleaning is enabled at all. This sort of suggests it controls whether it's created at a certain time as opposed to another time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rephrased.

@SparkQA
Copy link

SparkQA commented Nov 28, 2017

Test build #3995 has finished for PR 19826 at commit bd6c2bb.

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

</tr>
</table>

### Spark Application Garbage Collector
Copy link
Member

Choose a reason for hiding this comment

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

Pardon, I just noticed one more thing. This is really about the Context Cleaner, right? not garbage collection per se. I'd probably put these properties under Memory Management instead of a new section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved as asked.

@HyukjinKwon
Copy link
Member

retest this please

</tr>
</table>


Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am insane but let's revert unneeded change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

@SparkQA
Copy link

SparkQA commented Nov 30, 2017

Test build #84343 has finished for PR 19826 at commit d117c9f.

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

@HyukjinKwon
Copy link
Member

retest this please

@SparkQA
Copy link

SparkQA commented Nov 30, 2017

Test build #84358 has finished for PR 19826 at commit 241dbd0.

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

@srowen
Copy link
Member

srowen commented Dec 1, 2017

Merged to master

@asfgit asfgit closed this in 7e5f669 Dec 1, 2017
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