Fallback to eigentrust for SAW. Re-enable SAW & local test#1600
Fallback to eigentrust for SAW. Re-enable SAW & local test#1600ryle-goehausen wants to merge 2 commits intodevfrom
Conversation
| selfScores.foreach{case (k, v) => selfM(k) = v} | ||
| val addressToId = selfScores.keys.map{k => k.address -> k }.toMap | ||
| if (this.eigenTrust != null) { | ||
| val addressMap = this.eigenTrust.getTrustForAddresses.unsafeRunSync() |
There was a problem hiding this comment.
Please avoid unsafeRunSync. An effectful code should be wrapped into a monad that will delay the execution of side effects. Running unsafeRunSync manually is both unsafe and performance degrading.
There was a problem hiding this comment.
Fixed, this code is hopefully only temporary until larger fix implemented -- and should be fast since it's only accessing a map, but I rewrote it as monadic anyways.
| idMappedScores: Map[Id, Double] = sawAttempt match { | ||
| case Success(x) => x | ||
| case Failure(e) => | ||
| logger.error(s"SAW Failure: $e") |
There was a problem hiding this comment.
this logger will not work because it's wrapped in a monad
| sawAttempt = Try { | ||
| SelfAvoidingWalk | ||
| .runWalkFeedbackUpdateSingleNode( |
There was a problem hiding this comment.
sawAttempt <- F.delay { SelfAvoidingWalk.run... }.attempt // Either[Throwable, Map[Id, Double]
idMappedScores <- sawAttempt match {
case Right(x) => F.pure(x)
case Left(e) => logger.error(s"SAW Failure: $e") >>
...
this.eigenTrust.getTrustForAddresses.flatMap { addressMap =>
...
}
}
|
@marcinwadon re-ran scalafmt as well. Should I rebase? It seems like squash commits are required by default, I am happy to just use the commit message from original on a squash commit to avoid having to rebase every time. |
Yeah, linear history is required. You can either squash and keep the original message, or once updating the pull request, just ammend changes instead of creating a new commit. |
|
@marcinwadon ready for review again, just to clarify I usually use the 'Squash and Merge' option in PR page (this webpage) -- that is sufficient right? I think it's required anyways. I'll squash with original commit message |
2 participants
Tested this on cluster.