Skip to content

[CORE][MINOR] Correct the comment to show heartbeat interval is configurable#24101

Closed
SongYadong wants to merge 2 commits intoapache:masterfrom
SongYadong:heartbeat_interval_comment
Closed

[CORE][MINOR] Correct the comment to show heartbeat interval is configurable#24101
SongYadong wants to merge 2 commits intoapache:masterfrom
SongYadong:heartbeat_interval_comment

Conversation

@SongYadong
Copy link
Contributor

What changes were proposed in this pull request?

Executor heartbeat interval is configurable by "spark.executor.heartbeatInterval". But in a comment, heartbeat interval is presented as a constant 10s. This pr tries to correct the description.

How was this patch tested?

Existing unit tests.

* When an executor is unable to send heartbeats to the driver more than `HEARTBEAT_MAX_FAILURES`
* times, it should kill itself. The default value is 60. It means we will retry to send
* heartbeats about 10 minutes because the heartbeat interval is 10s.
* heartbeats about 10 minutes if the heartbeat interval is set to 10s or left default(10s).
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 the sentence still has problems. How about: "For example, if max failures is 60 and heartbeat interval is 10s, then it will try to send heartbeats for up to 600s (10 minutes)."

Copy link
Contributor Author

@SongYadong SongYadong Mar 15, 2019

Choose a reason for hiding this comment

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

yes, it seems better. thanks ! I'will update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated. @srowen

@maropu
Copy link
Member

maropu commented Mar 15, 2019

ok to test

@SparkQA
Copy link

SparkQA commented Mar 15, 2019

Test build #103531 has finished for PR 24101 at commit b629da6.

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

@srowen
Copy link
Member

srowen commented Mar 16, 2019

Merged to master

@srowen srowen closed this in ec11790 Mar 16, 2019
@SongYadong
Copy link
Contributor Author

SongYadong commented Mar 16, 2019

Thanks for review and merging. @srowen

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