[SPARK-27404][CORE][SQL][STREAMING][YARN] Fix build warnings for 3.0: postfixOps edition #24314
Conversation
…ings without the scala.language.postfixOps import
| ManagedBuffer block0 = new NioManagedBuffer(ByteBuffer.wrap(new byte[13])); | ||
| ManagedBuffer block1 = new NioManagedBuffer(ByteBuffer.wrap(new byte[7])); | ||
| ManagedBuffer block2 = new NioManagedBuffer(ByteBuffer.wrap(new byte[19])); | ||
| private final ManagedBuffer block0 = new NioManagedBuffer(ByteBuffer.wrap(new byte[13])); |
There was a problem hiding this comment.
OK, this is stuff I just cleaned up while touching this file.
|
|
||
| assertNotNull(stub); | ||
| stub.when(fetchStarter).createAndStart(any(), anyObject()); | ||
| stub.when(fetchStarter).createAndStart(any(), any()); |
There was a problem hiding this comment.
This was an avoidable deprecated method
| // Set a fixed timeout for RPC here, so users shall get a SparkException thrown by | ||
| // BarrierCoordinator on timeout, instead of RPCTimeoutException from the RPC framework. | ||
| timeout = new RpcTimeout(31536000 /* = 3600 * 24 * 365 */ seconds, "barrierTimeout")) | ||
| timeout = new RpcTimeout(365.days, "barrierTimeout")) |
There was a problem hiding this comment.
A good example of simplifying expressions while also removing postfixOps
| eventually(timeout(10.seconds), interval(1.second)) { | ||
| events.foreach { | ||
| case e: LoadInstanceStart[PipelineStage] | ||
| case e: LoadInstanceStart[_] |
There was a problem hiding this comment.
Can't match generic types - causes a warning
| } | ||
| } | ||
|
|
||
| abstract class JavaSimpleScanBuilder implements ScanBuilder, Scan, Batch { |
There was a problem hiding this comment.
These were moved to top-level classes. Multiple top-level classes in Java are mostly a no-no, and actually generate a warning if you try to use them in another file.
|
Test build #104361 has finished for PR 24314 at commit
|
|
Test build #104363 has finished for PR 24314 at commit
|
|
Wait until you see what has to happen to get rid of (mis)use of |
|
Test build #104364 has finished for PR 24314 at commit
|
| val rpcEndpointRef = remoteEnv.setupEndpointRef(localEnv.address, "send-authentication") | ||
| rpcEndpointRef.send("hello") | ||
| eventually(timeout(5 seconds), interval(10 millis)) { | ||
| eventually(timeout(5.seconds), interval(10.millis)) { |
There was a problem hiding this comment.
10.millis => 10.milliseconds
There are a few other places where this pattern is repeated:
(pr/24314) $ git diff 017919b636 | grep "^\+.*millis[^e]"
+ eventually(timeout(1.minute), interval(10.millis)) {
+ rpcEndpointRef.askSync[String]("hello", new RpcTimeout(1.millis, shortProp))
+ eventually(timeout(5.seconds), interval(10.millis)) {
+ eventually(timeout(1.minute), interval(200.millis)) {
+ Await.ready(f, 50.millis)
+ eventually(timeout(20.seconds), interval(batchDuration.milliseconds.millis)) {
+ eventually(timeout(10.seconds), interval(10.millis)) {
There was a problem hiding this comment.
Of course this is just a Nit as both correct (but from your previous changes I have the feeling milliseconds is prefered).
attilapiros
left a comment
attilapiros
left a comment
There was a problem hiding this comment.
I have run some completeness grep (for finding second/milli/minute without dot prefix outside of string literals and comments) and this PR seams to me complete.
Regarding removing the unnecessary case after map and foreach expressions there are still cases Like:
But as I got it that is not the scope of this change.
So LGTM.
|
Thanks @attilapiros yeah I fixed up more of those instances. Yes I have taken the liberty of also 'fixing' some minor things like unnecessary case statements etc in nearby code but tried to keep it mostly to build warnings and postfixOps here. |
|
Test build #104394 has finished for PR 24314 at commit
|
|
Test build #104409 has finished for PR 24314 at commit
|
|
Test build #104434 has started for PR 24314 at commit |
|
Test build #4700 has started for PR 24314 at commit |
|
Test build #4701 has started for PR 24314 at commit |
|
Test build #4702 has finished for PR 24314 at commit
|
|
Test build #4703 has started for PR 24314 at commit |
|
Retest this please. |
|
Test build #104509 has finished for PR 24314 at commit
|
|
Merged to master |
5 participants
What changes were proposed in this pull request?
Fix build warnings -- see some details below.
But mostly, remove use of postfix syntax where it causes warnings without the
scala.language.postfixOpsimport. This is mostly in expressions like "120000 milliseconds". Which, I'd like to simplify to things like "2.minutes" anyway.How was this patch tested?
Existing tests.