Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WX-735 Fix incorrect and/or nondeterministic filesystem ordering #6930

Merged
merged 9 commits into from
Oct 14, 2022
Merged

WX-735 Fix incorrect and/or nondeterministic filesystem ordering #6930

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cd80f9c
`SortedMap` shot in the dark
aednichols Sep 26, 2022
8f5d62e
Comment
aednichols Sep 26, 2022
bcfbc4d
Fix compiling
aednichols Oct 6, 2022
bd0b743
Merge remote-tracking branch 'origin/develop' into aen_wx_735
aednichols Oct 6, 2022
330d10f
Filesystem priority
aednichols Oct 11, 2022
261a4f5
Leave room for higher priority
aednichols Oct 11, 2022
217445d
Simplify sort
aednichols Oct 11, 2022
3608bfd
Implement constants suggestion
aednichols Oct 13, 2022
80500ac
Remove redundant comment
aednichols Oct 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/scala/cromwell/core/path/PathBuilderFactory.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ object PathBuilderFactory {
val sortedFactories = factories.sortWith({
case (_, DefaultPathBuilderFactory) => true
case (DefaultPathBuilderFactory, _) => false
case (a, b) => factories.indexOf(a) < factories.indexOf(b)
case (a, b) => factories.indexOf(a) < factories.indexOf(b) // retains pre-existing order of `factories`
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL that we already impose an order on these, that makes me feel better.

What initially sets this order? Is it the order the filesystems appear in the config file?

})
sortedFactories.traverse(_.withOptions(workflowOptions))
}
Expand Down
14 changes: 9 additions & 5 deletions engine/src/main/scala/cromwell/engine/EngineFilesystems.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,23 @@ import cromwell.core.filesystem.CromwellFileSystems
import cromwell.core.path.{DefaultPathBuilderFactory, PathBuilder, PathBuilderFactory}
import net.ceedubs.ficus.Ficus._

import scala.collection.immutable.SortedMap
import scala.concurrent.Future

object EngineFilesystems {
private val config: Config = ConfigFactory.load

private val defaultFileSystemFactory: Map[String, PathBuilderFactory] =
private val defaultFileSystemFactory: SortedMap[String, PathBuilderFactory] =
Option(DefaultPathBuilderFactory.tuple)
.filter(_ => config.as[Boolean]("engine.filesystems.local.enabled"))
.toMap
.to(collection.immutable.SortedMap)

private val pathBuilderFactories: Map[String, PathBuilderFactory] = {
CromwellFileSystems.instance.factoriesFromConfig(config.as[Config]("engine"))
.unsafe("Failed to instantiate engine filesystem") ++ defaultFileSystemFactory
private val pathBuilderFactories: SortedMap[String, PathBuilderFactory] = {
// Unordered maps are a classical source of randomness injection into a system
(
CromwellFileSystems.instance.factoriesFromConfig(config.as[Config]("engine"))
.unsafe("Failed to instantiate engine filesystem") ++ defaultFileSystemFactory
).to(collection.immutable.SortedMap)
}

def configuredPathBuilderFactories: List[PathBuilderFactory] = pathBuilderFactories.values.toList
Expand Down