Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Scala 2.12 support for Cromwell. Closes #1635 #2412
Conversation
| + MetadataQuery(_, Some(_), None, Some(_), Some(_), _) | | ||
| + MetadataQuery(_, Some(_), Some(_), None, Some(_), _) | | ||
| + MetadataQuery(_, Some(_), Some(_), Some(_), None, _) | | ||
| + MetadataQuery(_, Some(_), Some(_), Some(_), Some(_), _) => Future.failed(new IllegalStateException("Invalid MetadataQuery")) |
kshakir
Jul 5, 2017
Contributor
- How about
new IllegalStateException(s"Invalid MetadataQuery $query") - Is a singular
case _ =>too loose for your liking?
geoffjentry
Jul 5, 2017
Member
- sure
- Presumably it'd satisfy the compiler, so sure. I just copy/pasted that from the error message w/o thinking about it too much (other than 'yuck') :)
|
|
cjllanwarne
reviewed
Jul 5, 2017
I also spotted a 2.11 in the README.md under requirements. Was that deliberately unchanged?
| import scala.reflect.{ClassTag, classTag} | ||
| object ConfigUtil { | ||
| val validationLogger = LoggerFactory.getLogger("ConfigurationValidation") | ||
| implicit class EnhancedConfig(val config: Config) extends AnyVal { | ||
| - def keys = config.entrySet().toSet map { v: java.util.Map.Entry[String, ConfigValue] => v.getKey } | ||
| + def keys = config.entrySet().asScala.toSet map { v: java.util.Map.Entry[String, ConfigValue] => v.getKey } |
cjllanwarne
Jul 5, 2017
Contributor
Is this right? Won't the asScala remove any Java-y types from the map?
geoffjentry
Jul 5, 2017
Member
I believe it just converts to a scala collection.
The key here is that the JavaConversions are gone now, so JavaConverters is the only thing available w/o doing it by hand.
geoffjentry
Jul 5, 2017
Member
w/o asScala: util.Set[util.Map.Entry[String, ConfigValue]]
w/ asScala: mutable.Set[util.Map.Entry[String, ConfigValue]]
Note that the trailing toSet sets it to be immutable
cjllanwarne
Jul 5, 2017
Contributor
Oh I think I see... the config.entrySet is a (pardon the syntax) JavaSet[JavaMap.Entry] and asScala makes it a ScalaSet[JavaMap.Entry]. LGTM
| @@ -336,7 +336,7 @@ class CromwellApiServiceSpec extends AsyncFlatSpec with ScalatestRouteTest with | ||
| check { | ||
| status should be(StatusCodes.OK) | ||
| val decoder: Decoder = Gzip | ||
| - val result = Await.result(Unmarshal(decoder.decode(response)).to[JsObject], 1.second) | ||
| + val result = Await.result(Unmarshal(decoder.decodeMessage(response)).to[JsObject], 1.second) |
cjllanwarne
Jul 5, 2017
Contributor
I think this fits the pattern that lets you just say val result = responseAs[JsObject], avoiding the whole convoluted Await.result and decoding boilerplate
(and the examples below too, if it works here)
cjllanwarne
Jul 5, 2017
Contributor
cf the comment near the end of my lightbend support ticket:
AKKA HTTP: responding using a Stream in high-level DSL
geoffjentry
Jul 5, 2017
Member
IIRC there was a reason why I couldn't do that in the first place (I think I used responseAs elsewhere in the file, I definitely played around with it). And by "couldn't", i probably mean "that was going to be a giant pain in the ass for some reason I didn't want to deal with it at the time".
I'll take another poke.
geoffjentry
Jul 5, 2017
Member
oh duh. it's because i couldn't see a way to use responseAs with gzip
| case AbortedWorkflowId => | ||
| WorkflowAbortFailed(id, new IllegalStateException(s"Workflow ID '$id' is in terminal state 'Aborted' and cannot be aborted.")) | ||
| + case WorkflowId(_) => throw new Exception("Something untoward happened") |
cjllanwarne
Jul 5, 2017
Contributor
ToL: I like test exceptions to be obviously distinguishable between "don't worry, I wanted this to happen for the test" vs "your test has broken". It's not obvious to me which this falls into.
geoffjentry
Jul 5, 2017
Member
This is "clearly the test author didn't think this could possibly happen, so this should probably blow up: :)
| @@ -57,7 +57,7 @@ object GoogleConfiguration { | ||
| private val log = LoggerFactory.getLogger("GoogleConfiguration") | ||
| - case class GoogleConfigurationException(errorMessages: List[String]) extends MessageAggregation { | ||
| + final case class GoogleConfigurationException(errorMessages: List[String]) extends MessageAggregation { |
| - // TODO: Re-combine these when cromwell is 2.12: | ||
| - lazy val cromwellApiClientAkkaV = "2.4.17" | ||
| - lazy val cromwellApiClientAkkaHttpV = "10.0.6" | ||
| + |
| @@ -125,6 +125,7 @@ trait MetadataDatabaseAccess { | ||
| case MetadataQuery(_, None, None, Some(includeKeys), Some(excludeKeys), _) => Future.failed( | ||
| new IllegalArgumentException( | ||
| s"Include/Exclude keys may not be mixed: include = $includeKeys, exclude = $excludeKeys")) | ||
| + case _ => Future.failed(new IllegalStateException(s"Invalid MetadataQuery: $query")) |
cjllanwarne
Jul 5, 2017
Contributor
ToL: Any reason for InvalidStateException rather than the InvalidArgumentException elsewhere in the file?
geoffjentry
Jul 5, 2017
Member
I chose InvalidStateException because this is covering a case which the developer(s) seemed to believe wasn't possible (and clearly has never happened before since it hasn't come up).
The 2.12 linter is more robust to detecting non-exhaustive matching
geoffjentry
Jul 5, 2017
Member
although now that i look at it i realize that this is coming effectively from the user
| @@ -60,7 +60,7 @@ object JesAttributes { | ||
| implicit val urlReader: ValueReader[URL] = StringReader.stringValueReader.map { URI.create(_).toURL } | ||
| def apply(googleConfig: GoogleConfiguration, backendConfig: Config): JesAttributes = { | ||
| - val configKeys = backendConfig.entrySet().toSet map { entry: java.util.Map.Entry[String, ConfigValue] => entry.getKey } | ||
| + val configKeys = backendConfig.entrySet().asScala.toSet map { entry: java.util.Map.Entry[String, ConfigValue] => entry.getKey } |
| @@ -11,7 +10,7 @@ object JesJobPaths { | ||
| val GcsExecPathKey = "gcsExec" | ||
| } | ||
| -case class JesJobPaths(override val workflowPaths: JesWorkflowPaths, jobKey: BackendJobDescriptorKey)(implicit actorSystem: ActorSystem) extends JobPaths { | ||
| +case class JesJobPaths(override val workflowPaths: JesWorkflowPaths, jobKey: BackendJobDescriptorKey) extends JobPaths { |
|
@cjllanwarne good catch on the 2.11, found a few others. I think i might have done a find on |
|
|
| - (implicit ec: ExecutionContext): Future[StandardAsyncRunStatus] = { | ||
| - Future.fromTry(Try(pollStatus(handle))) | ||
| - } | ||
| + def pollStatusAsync(handle: StandardAsyncPendingExecutionHandle): Future[StandardAsyncRunStatus] = Future.fromTry(Try(pollStatus(handle))) |
mcovarr
Jul 5, 2017
Contributor
the Try => Future thing happens three times just in this file, perhaps some sugar is in order
geoffjentry
Jul 5, 2017
Member
I don't even understand why it's doing it this way in the first place instead of just a future.
that said, i'll add it to the list of preexisting conditions i claim i'm going to fix from my akka http list
geoffjentry
Jul 5, 2017
Member
Of course then I'd need to rollback my removal of the ECs, oh the tangled webs we weave
| + The following aren't used (yet), and in general are an exercise in pain for 2.12 with Cromwell. They'd | ||
| + certainly be nice to have, but params causes a world of hurt and patvars is just going to be a big time sink. |
mcovarr
Jul 5, 2017
Contributor
holy moly, it'll be a time sink just understanding all the ones that did get turned on
geoffjentry
Jul 5, 2017
Member
You don't want to know how much time I spent over the weekend before I discovered I could just turn on everything but the stuff I wanted in there.
geoffjentry
Jul 5, 2017
Member
BTW if you want to get an idea, look at the branch jg_scala_2_12, and I didn't even manage to fix all the things those two things were complaining about before I gave up
| @@ -42,7 +42,7 @@ mysql -u root -e "GRANT ALL PRIVILEGES ON cromwell_test . * TO 'travis'@'localho | ||
| WORKDIR=$(pwd) | ||
| sbt assembly | ||
| -CROMWELL_JAR=$(find "$(pwd)/target/scala-2.11" -name "cromwell-*.jar") | ||
| +CROMWELL_JAR=$(find "$(pwd)/target/scala-2.12" -name "cromwell-*.jar") |
mcovarr
Jul 5, 2017
Contributor
which reminds me we should check what the release WDL is expecting WRT 2.11 / 2.12
|
|
|
geoffjentry |
All checks have passed |
37370c8
|
geoffjentry commentedJul 4, 2017
No description provided.