Skip to content

Conversation

@original-brownbear
Copy link
Member

What changes were proposed in this pull request?

  1. Removing all redundant throws declarations from Java codebase.
  2. Removing dead code made visible by this from ShuffleExternalSorter#closeAndGetSpills

How was this patch tested?

Build still passes.

@SparkQA
Copy link

SparkQA commented Sep 10, 2017

Test build #3915 has finished for PR 19182 at commit 3181f69.

  • This patch fails to generate documentation.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Sep 11, 2017

Some of these 'throws' clauses may not be removable because they cause callers that catch the checked exception to fail to compile.

Removing "throws Exception" in tests isn't obviously helpful, because it means the signature has to change any time the tested code happens to throw a new checked exception. There's no API problem as nothing invokes these methods directly.

Private non-test methods, maybe.

@original-brownbear original-brownbear force-pushed the SPARK-21970 branch 2 times, most recently from a5b0d31 to b69a20b Compare September 11, 2017 11:17
@original-brownbear
Copy link
Member Author

@srowen makes perfect sense => rolled back all changes to tests + publicly exposed methods (those package private ones adjusted are on non-public classes).

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

This kind of thing looks like good cleanup if it compiles and passes MiMa

@SparkQA
Copy link

SparkQA commented Sep 11, 2017

Test build #3916 has finished for PR 19182 at commit b69a20b.

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

@original-brownbear
Copy link
Member Author

@srowen looks like we're all green :)

@srowen
Copy link
Member

srowen commented Sep 12, 2017

Ah OK one more subtle thing @original-brownbear -- the code you see in org/apache/hive packages is, I believe, copied from Hive. Therefore it's probably best to leave it as-is because it makes it easier to update it if it hasn't varied at all from its source. Could you reverse those? otherwise looks OK.

@original-brownbear
Copy link
Member Author

@srowen done, all changes to org.apache.hive.* reverted :)

@SparkQA
Copy link

SparkQA commented Sep 13, 2017

Test build #3921 has finished for PR 19182 at commit cb8e80b.

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

@srowen
Copy link
Member

srowen commented Sep 13, 2017

Merged to master

@asfgit asfgit closed this in b6ef1f5 Sep 13, 2017
@original-brownbear original-brownbear deleted the SPARK-21970 branch September 13, 2017 13:14
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