Skip to content

Conversation

@10110346
Copy link
Contributor

@10110346 10110346 commented Jul 20, 2018

What changes were proposed in this pull request?

This pr add spark.broadcast.checksum to configuration.

How was this patch tested?

manually tested

@SparkQA
Copy link

SparkQA commented Jul 20, 2018

Test build #93329 has finished for PR 21825 at commit 6a85aad.

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

@srowen
Copy link
Member

srowen commented Jul 27, 2018

Are you sure this is supposed to be documented for end users?

@10110346
Copy link
Contributor Author

10110346 commented Jul 30, 2018

I think it's necessary to let the end users know:
1.this feature is already stable
2.People can disable it if their network is stable, doing this is good for performance
@srowen thanks

Copy link
Member

Choose a reason for hiding this comment

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

If this is going to be documented, it should be documented in a way that's useful for end users. This typically means describing what turning it on and off does from an end-user perspective. Does it make things faster, slower? more reliable, compatible, etc? This description here pretty much reiterates what's in the property key itself.

@SparkQA
Copy link

SparkQA commented Jul 31, 2018

Test build #93823 has finished for PR 21825 at commit cf27272.

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

Copy link
Member

Choose a reason for hiding this comment

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

Nits like: space after a period, and the default is already documented above. I think this could still be more useful. What about: "If enabled, broadcasts will include a checksum, which can help detect corrupted blocks, at the cost of computing and sending a little more data. It's possible to disable it if the network has other mechanisms to guarantee data won't be corrupted during broadcast."

CC @davies . I guess even I'm not sure when I would disable this ... what would a network have to guarantee to avoid whatever corruption is possible here? Here it isn't clear yet when it's safe, when it won't lead to correctness issues.

@SparkQA
Copy link

SparkQA commented Aug 1, 2018

Test build #93876 has finished for PR 21825 at commit d18d1b2.

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

Copy link
Member

Choose a reason for hiding this comment

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

Final nit: add the space after the period.

@SparkQA
Copy link

SparkQA commented Aug 2, 2018

Test build #93919 has finished for PR 21825 at commit 09cbe26.

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

@srowen
Copy link
Member

srowen commented Aug 2, 2018

Merged to master

@asfgit asfgit closed this in c5fe412 Aug 2, 2018
@10110346
Copy link
Contributor Author

10110346 commented Aug 2, 2018

Thanks @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.

3 participants