Adding Scan Connection persistence to validator #3197
Conversation
[ci] Signed-off-by: Pasindu Tennage <pasindu.tennage@digitalasset.com>
[ci] Signed-off-by: Pasindu Tennage <pasindu.tennage@digitalasset.com>
[ci] Signed-off-by: Pasindu Tennage <pasindu.tennage@digitalasset.com>
[ci] Signed-off-by: Pasindu Tennage <pasindu.tennage@digitalasset.com>
[ci] Signed-off-by: Pasindu Tennage <pasindu.tennage@digitalasset.com>
[ci] Signed-off-by: Pasindu Tennage <pasindu.tennage@digitalasset.com>
|
Is |
martinflorian-da
left a comment
martinflorian-da
left a comment
There was a problem hiding this comment.
Looks reasonable to me so far!
Any reason not do add a test right away? I'm always a bit hesitant to add new code paths that are not tested, even if we plan to follow up with a test.
(LMK if you need ideas for how to design an integration test here. I think I can generate some plausible ideas.)
| ): Future[BftScanConnection] = { | ||
| val logger = loggerFactory.getTracedLogger(getClass) | ||
|
|
||
| val persistedUris: NonEmptyList[Uri] = lastPersistedScanUrlList match { |
There was a problem hiding this comment.
| val persistedUris: NonEmptyList[Uri] = lastPersistedScanUrlList match { | |
| val bootstrapUris: NonEmptyList[Uri] = lastPersistedScanUrlList match { |
(turbo-)nit
nicu-da
left a comment
nicu-da
left a comment
There was a problem hiding this comment.
Thanks, great work so far
| protected val initialScanConnections: Seq[SingleScanConnection] | ||
| protected val initialFailedConnections: Map[Uri, Throwable] | ||
| protected val connectionBuilder: Uri => Future[SingleScanConnection] | ||
| protected val persistScanUrlCallback: Option[Seq[DsoScan] => Future[Unit]] |
There was a problem hiding this comment.
| protected val persistScanUrlCallback: Option[Seq[DsoScan] => Future[Unit]] | |
| protected val refreshScanUrlsCallback: Option[Seq[DsoScan] => Future[Unit]] |
the fact that we persist them as part of that callback doesn't matter here
| retryProvider: RetryProvider, | ||
| loggerFactory: NamedLoggerFactory, | ||
| builder: (Uri, NonNegativeFiniteDuration) => Future[SingleScanConnection], | ||
| lastPersistedScanUrlList: Option[Seq[DsoScan]], |
There was a problem hiding this comment.
This shouldn't leak here, it should just be passed as the seed urls.
| initialSynchronizerTime <- | ||
| withParticipantAdminConnection { participantAdminConnection => | ||
| for { | ||
| _ <- Future.unit |
There was a problem hiding this comment.
this is a bit confusing, I would just move the internalStore definition outside the for in this case
|
|
||
| val maybeInternalConfigs: Option[Seq[ScanUrlInternalConfig]] = | ||
| try { | ||
| Await.result(futureOption, 10.seconds) // what is the max waiting time? |
There was a problem hiding this comment.
We should never wait for futures in non test code. We should always include them in a for comprehension as we already run everything more or less in an async context
| try { | ||
| Await.result(futureOption, 10.seconds) // what is the max waiting time? | ||
| } catch { | ||
| case e: Throwable => |
There was a problem hiding this comment.
Let's avoid catch all scenarios.
Note: in scala catching throwable would also catch scala flow control exceptions such as return. Worst case scenario we should use catch NonFatal. Also we should never silently ignore exceptions
Adding support for validator to save the scan urls in the DB
Fixes https://github.com/issues/assigned?issue=DACH-NY%7Ccanton-network-internal%7C451