Switch to Scala 2.12.7 #4062

chetanmeh · 2018-10-10T18:27:04Z

Updates the Scala compiler and library to 2.12.7

Description

Apart from switching the jar versions to 2.12 some code changes were required to avoid use of now deprecated features.

Avoid ambiguty between global executionContext and local one
Use explicit eta expansion instead of implicit one
Use JavaConverters over JavaConversion
Remove use of deprecated future methods

Related issue and scope

I opened an issue to propose and discuss this change (Update Scala to 2.12 #3952)

My changes affect the following components

Types of changes

Bug fix (generally a non-breaking change which closes an issue).
Enhancement or new feature (adds new functionality).
Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

I signed an Apache CLA.
I reviewed the style guides and followed the recommendations (Travis CI will check :).
I added tests to cover my changes.
My changes require further changes to the documentation.
I updated the documentation where necessary.

rabbah

LGTM

markusthoemmes

Thanks a lot for doing this, got one minor nit which I think should be addressed though.

markusthoemmes · 2018-10-10T18:41:48Z

common/scala/src/main/scala/whisk/core/database/memory/MemoryArtifactStore.scala

-    f.onSuccess({
-      case _ => transid.finished(this, start, s"[PUT] '$dbName' completed document: '$docinfoStr'")
-    })
+    f.foreach(_ => transid.finished(this, start, s"[PUT] '$dbName' completed document: '$docinfoStr'"))


Can we use onComplete everywhere there's the success/failure case?

chetanmeh · 2018-10-11T04:40:40Z

Compilation time for whole project via reduced from 2 min 39 sec to 1 min 46 sec, thus a 33% improvement !!

$./gradlew clean
$./gradlew compileScala compileTestScala

chetanmeh · 2018-10-11T04:47:49Z

Compilation time is reduced specially for test module

2.11	2.12

codecov-io · 2018-10-11T05:12:42Z

Codecov Report

Merging #4062 into master will increase coverage by 4.51%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #4062      +/-   ##
==========================================
+ Coverage   76.63%   81.14%   +4.51%     
==========================================
  Files         148      148              
  Lines        7249     7113     -136     
  Branches      445      439       -6     
==========================================
+ Hits         5555     5772     +217     
+ Misses       1694     1341     -353

Impacted Files	Coverage Δ
...scala/whisk/core/mesos/MesosContainerFactory.scala	`61.81% <ø> (ø)`	⬆️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	`0% <0%> (-95.54%)`	⬇️
...on/scala/src/main/scala/whisk/common/Logging.scala	`89.58% <0%> (+2.21%)`	⬆️
...n/scala/whisk/core/database/CouchDbRestStore.scala	`73.23% <100%> (ø)`	⬆️
...ala/whisk/core/database/s3/S3AttachmentStore.scala	`85.71% <100%> (ø)`	⬆️
.../src/main/scala/whisk/core/connector/Message.scala	`64.86% <100%> (+7.33%)`	⬆️
...rc/main/scala/whisk/core/database/StoreUtils.scala	`100% <100%> (ø)`	⬆️
...cala/src/main/scala/whisk/core/entity/Limits.scala	`84.61% <100%> (ø)`	⬆️
...core/controller/BasicAuthenticationDirective.scala	`100% <100%> (+42.85%)`	⬆️
...isk/core/database/memory/MemoryArtifactStore.scala	`97.54% <83.33%> (-0.84%)`	⬇️
... and 61 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f47410d...5262791. Read the comment docs.

chetanmeh · 2018-10-11T06:21:57Z

Before we merge this to master we need to have PR for all other OpenWhisk repo relying on core repo and then coordinate the merge across all those repos

chetanmeh · 2018-10-11T09:18:51Z

Created 16 PR 😰 and no code change needed in other repos. Of 16 build is failing for 4 for reasons not related to this change.

Once this PR is merged those 16 PR would need to be updated to point to latest master and then merged

chetanmeh · 2018-10-11T09:32:20Z

@vvraskin @cbickel Can you check if your downstream project can compile fine with this change. If required you can use the PR branch

git clone --depth=1 --single-branch -b scala-2-12 https://github.com/chetanmeh/incubator-openwhisk.git openwhisk

markusthoemmes · 2018-10-11T12:35:55Z

Kudos @chetanmeh this is a lot of repetitive work. Thanks a ton for pulling through!

chetanmeh · 2018-10-18T05:16:12Z

@csantanapr I have reverted all git branch change from various PR. So all PR now only have required changes for Scala 2.12 support.

csantanapr · 2018-11-02T20:38:45Z

In IBM we did some refactoring in our internal repos and public repos for this and we are ready for the Scala bump.

csantanapr · 2018-11-02T20:39:01Z

The conflict needs to be resolved before we can start the merge fest.

Change all scala based libs to 2.12

…tion

See https://viktorklang.com/blog/Futures-in-Scala-2.12-part-5.html

…tion

https://stackoverflow.com/questions/40442717/implicit-executioncontext-priority-in-scala-2-12

chetanmeh · 2018-11-06T08:32:25Z

Post rebase the invoker is throwing exception at runtime

Exception in thread "pool-1-thread-3" java.lang.AbstractMethodError: Method whisk/core/connector/CompletionMessage.whisk$core$connector$Message$_setter_$transid_$eq(Lwhisk/common/TransactionMetadata;)V is abstract
	at whisk.core.connector.CompletionMessage.whisk$core$connector$Message$_setter_$transid_$eq(Message.scala)
	at whisk.core.connector.Message.$init$(Message.scala:31)
	at whisk.core.connector.AcknowledegmentMessage.<init>(Message.scala:78)
	at whisk.core.connector.CompletionMessage.<init>(Message.scala:93)
	at whisk.core.invoker.InvokerReactive.whisk$core$invoker$InvokerReactive$$send$1(InvokerReactive.scala:128)
	at whisk.core.invoker.InvokerReactive.$anonfun$ack$1(InvokerReactive.scala:149)
	at whisk.core.invoker.InvokerReactive.$anonfun$ack$1$adapted(InvokerReactive.scala:121)
	at whisk.core.containerpool.ContainerProxy.$anonfun$initializeAndRun$15(ContainerProxy.scala:428)
	at scala.util.Success.foreach(Try.scala:249)
	at scala.concurrent.Future.$anonfun$foreach$1$adapted(Future.scala:225)
	at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:60)
	at akka.dispatch.BatchingExecutor$AbstractBatch.processBatch(BatchingExecutor.scala:55)
	at akka.dispatch.BatchingExecutor$BlockableBatch.$anonfun$run$1(BatchingExecutor.scala:91)
	at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:12)
	at scala.concurrent.BlockContext$.withBlockContext(BlockContext.scala:81)
	at akka.dispatch.BatchingExecutor$BlockableBatch.run(BatchingExecutor.scala:91)
	at akka.dispatch.TaskInvocation.run(AbstractDispatcher.scala:40)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

Strangely compilation passes fine. The transid in Message is getting marked as abstract even though its provided in case class parameters

Seems like this was changed in #4041 and may be a bug in Scala compiler. Needs further investigation

chetanmeh · 2018-11-06T08:44:21Z

The issue can be seen with this fiddle. It passes with 2.11 but fails with 2.12. Probably related to scala/bug#10477

chetanmeh · 2018-11-06T11:01:07Z

common/scala/src/main/scala/whisk/core/connector/Message.scala

@@ -75,8 +75,8 @@ object ActivationMessage extends DefaultJsonProtocol {
 * Message that is sent from the invoker to the controller after action is completed or after slot is free again for
 * new actions.
 */
-abstract class AcknowledegmentMessage() extends Message {
-  override val transid: TransactionId
+abstract class AcknowledegmentMessage(private val tid: TransactionId) extends Message {


Had to change code here to workaround Scala compiler bug (see previous comments)

cc @cbickel

chetanmeh · 2018-11-06T11:01:41Z

@csantanapr The PR is green now. So we can now do the merge

csantanapr · 2018-11-06T16:16:04Z

thanks @chetanmeh 👍

chetanmeh · 2018-11-07T09:52:23Z

Thanks @csantanapr for taking care of merge and changes in all other repos

rabbah approved these changes Oct 10, 2018

View reviewed changes

markusthoemmes approved these changes Oct 10, 2018

View reviewed changes

chetanmeh added the wip label Oct 11, 2018

chetanmeh removed the wip label Oct 11, 2018

chetanmeh force-pushed the scala-2-12 branch from ea88b0e to 5cf62e5 Compare October 15, 2018 12:52

This was referenced Oct 15, 2018

Scala 2.12.7 bump ibm-functions/runtime-nodejs#147

Merged

bump scala 2.12.7 ibm-functions/runtime-swift#54

Merged

This was referenced Oct 15, 2018

Scala bump 2 12 7 ibm-functions/package-push-notifications#26

Merged

scala bump 2.12.7 ibm-functions/template-hello-world#18

Merged

chetanmeh force-pushed the scala-2-12 branch from 5cf62e5 to 651f381 Compare October 18, 2018 04:39

chetanmeh mentioned this pull request Oct 18, 2018

Prepare for switch to Scala 2.12.7 #4074

Closed

21 tasks

chetanmeh added 12 commits November 6, 2018 10:47

Switch to Scala 2.12.7

ac615bc

Change all scala based libs to 2.12

Perform explicit eta expansion to disambiguate passing method as func…

5f4398a

…tion

Ensure case match covers all cases for LogLevel

db9bd1f

Use mesos-actor compiled with 2.12

b70e2cd

Refactor use of deprecated future methods

9cf1122

See https://viktorklang.com/blog/Futures-in-Scala-2.12-part-5.html

Perform explicit eta expansion to disambiguate passing method as func…

28f30bc

…tion

Remove unused imports

8f0ca53

Resolve ambiguity between gloabl and local execution context

b24dfb6

https://stackoverflow.com/questions/40442717/implicit-executioncontext-priority-in-scala-2-12

Switch to JavaConverters

5295c80

Use Java ForkJoinPool

5f9f8c9

Use onComplete for scenario having both success and failure handling

bd9f3f1

Scala 2.12 in travis

686849c

chetanmeh force-pushed the scala-2-12 branch from 651f381 to 686849c Compare November 6, 2018 05:20

Workaround issue related to initializing trait val in case class

5262791

chetanmeh commented Nov 6, 2018

View reviewed changes

csantanapr merged commit b860a2e into apache:master Nov 6, 2018

chetanmeh deleted the scala-2-12 branch November 7, 2018 09:52

BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019

Switch to Scala 2.12.7 (apache#4062)

4dc2de9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to Scala 2.12.7 #4062

Switch to Scala 2.12.7 #4062

chetanmeh commented Oct 10, 2018 •

edited

Loading

rabbah left a comment

markusthoemmes left a comment

markusthoemmes Oct 10, 2018

chetanmeh Oct 11, 2018

chetanmeh commented Oct 11, 2018

chetanmeh commented Oct 11, 2018

codecov-io commented Oct 11, 2018 •

edited

Loading

chetanmeh commented Oct 11, 2018

chetanmeh commented Oct 11, 2018

chetanmeh commented Oct 11, 2018

markusthoemmes commented Oct 11, 2018

chetanmeh commented Oct 18, 2018

csantanapr commented Nov 2, 2018

csantanapr commented Nov 2, 2018

chetanmeh commented Nov 6, 2018

chetanmeh commented Nov 6, 2018

chetanmeh Nov 6, 2018 •

edited

Loading

chetanmeh commented Nov 6, 2018

csantanapr commented Nov 6, 2018

chetanmeh commented Nov 7, 2018

Switch to Scala 2.12.7 #4062

Switch to Scala 2.12.7 #4062

Conversation

chetanmeh commented Oct 10, 2018 • edited Loading

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

rabbah left a comment

Choose a reason for hiding this comment

markusthoemmes left a comment

Choose a reason for hiding this comment

markusthoemmes Oct 10, 2018

Choose a reason for hiding this comment

chetanmeh Oct 11, 2018

Choose a reason for hiding this comment

chetanmeh commented Oct 11, 2018

chetanmeh commented Oct 11, 2018

codecov-io commented Oct 11, 2018 • edited Loading

Codecov Report

chetanmeh commented Oct 11, 2018

chetanmeh commented Oct 11, 2018

chetanmeh commented Oct 11, 2018

markusthoemmes commented Oct 11, 2018

chetanmeh commented Oct 18, 2018

csantanapr commented Nov 2, 2018

csantanapr commented Nov 2, 2018

chetanmeh commented Nov 6, 2018

chetanmeh commented Nov 6, 2018

chetanmeh Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

chetanmeh commented Nov 6, 2018

csantanapr commented Nov 6, 2018

chetanmeh commented Nov 7, 2018

chetanmeh commented Oct 10, 2018 •

edited

Loading

codecov-io commented Oct 11, 2018 •

edited

Loading

chetanmeh Nov 6, 2018 •

edited

Loading