[MINOR] Clean up several build warnings, mostly due to internal use of old accumulators #13642
Conversation
| test("Normal accumulator should do boxing") { | ||
| // We need this test to make sure BoxingFinder works. | ||
| val l = sparkContext.accumulator(0L) | ||
| val f = () => { l += 1L } |
There was a problem hiding this comment.
This assertion is no longer true in the new LongAccumulator API
There was a problem hiding this comment.
should we remove all the boxing test, include the BoxingFinder?
There was a problem hiding this comment.
Maybe... I am not sure what the purpose of the test was. The one that remains still passes.
BTW the old accumulator API is definitely still tested. I'm just changing tests that need an accumulator, not the old accumulator specifically. This avoids deprecation warnings.
There was a problem hiding this comment.
Then we should still keep this test, which is actually testing the BoxingFinder.
There was a problem hiding this comment.
There is no boxing in the new implementation, since LongAccumulator exposes a method that accepts a primitive long now. Of course, I could change the test to manually box it. But then that seems like it's defeating whatever purpose I can imagine for the test. Is that really the right thing to do?
There was a problem hiding this comment.
hmmm, since boxing is not a problem for the new accumulator anymore, maybe we can just remove all these tests? It's weird if we only remove one of them.
There was a problem hiding this comment.
Yeah, right now it's testing that a different, custom accumulator doesn't box. It doesn't, so it succeeds. You added the test so I think you're the authority here, and if you're OK removing the test, seems OK by me.
|
Test build #60404 has finished for PR 13642 at commit
|
|
maybe we should add a new test for old accumulators, to keep backward compatibility? |
|
Test build #60480 has finished for PR 13642 at commit
|
|
thanks, merging to master! @srowen , this PR conflicts with branch 2.0, can you send a new PR against 2.0? thanks! |
…f old accumulators Another PR to clean up recent build warnings. This particularly cleans up several instances of the old accumulator API usage in tests that are straightforward to update. I think this qualifies as "minor". Jenkins Author: Sean Owen <[email protected]> Closes #13642 from srowen/BuildWarnings. (cherry picked from commit 6151d26) Signed-off-by: Sean Owen <[email protected]>
|
I just cherry-picked this commit to 2.0 and resolved conflicts. |
| class SQLMetricsSuite extends SparkFunSuite with SharedSQLContext { | ||
| import testImplicits._ | ||
|
|
||
| test("SQLMetric should not box Long") { |
There was a problem hiding this comment.
Why remove this test? This test doesn't use the old accumulator API.
There was a problem hiding this comment.
This is not a problem anymore for new accumulator
There was a problem hiding this comment.
@cloud-fan the test here is just making sure we won't bring boxing into SQLMetric in future.
There was a problem hiding this comment.
Yeah, that was my logic. This wouldn't happen without breaking the new API, so it didn't seem worth testing for. Or: there are a million things of that form we could test for but don't.
There was a problem hiding this comment.
This wouldn't happen without breaking the new API,
Agreed. SQLMetric is not a public API, we may add new stuff. My point is that we don't need to delete a working test that does test some behavior we want to maintain. Anyway, this seems unlikely broken in future so I agree that not need to add it back.
4 participants
What changes were proposed in this pull request?
Another PR to clean up recent build warnings. This particularly cleans up several instances of the old accumulator API usage in tests that are straightforward to update. I think this qualifies as "minor".
How was this patch tested?
Jenkins