Add "initEnabled" config to DX #507
Conversation
awrichar
force-pushed
the
dxinit
branch
2 times, most recently
from
6d0666c to
aa4d812
Compare
Codecov Report
@@ Coverage Diff @@
## main #507 +/- ##
===========================================
- Coverage 100.00% 99.98% -0.02%
===========================================
Files 267 275 +8
Lines 15270 15725 +455
===========================================
+ Hits 15270 15723 +453
- Misses 0 1 +1
- Partials 0 1 +1
Continue to review full report at Codecov.
|
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
|
Not sure why coverage is showing a drop in dxhttps, as that file was previously removed. Also @peterbroadhurst identified an additional follow-on, in that the plugin should specifically reject all requests during the window that it is disconnected from the websocket (to avoid sending a request to a non-initialized DX). Can be added here or done as a new PR. |
peterbroadhurst
left a comment
peterbroadhurst
left a comment
There was a problem hiding this comment.
Would like to get your spelling thoughts before we merge @awrichar
However, the code looks great
internal/dataexchange/ffdx/config.go
Outdated
| // DataExchangeManifestEnabled determines whether to require+validate a manifest from other DX instances in the network. Must be supported by the connector | ||
| DataExchangeManifestEnabled = "manifestEnabled" | ||
| // DataExchangeReInitEnabled instructs FireFly to always post all current nodes to the /init API before connecting or reconnecting to the connector | ||
| DataExchangeReInitEnabled = "reInitEnabled" |
There was a problem hiding this comment.
I think this should be initEnabled - as you should always set this whenever you have a ffdx plugin that supports init. So we don't need to indicate the re (which is a bit confusing)
There was a problem hiding this comment.
Another spelling could be:
"apiVersion": "1.1"
There was a problem hiding this comment.
I think initEnabled is appropriate. Personally I think it should be independently toggleable regardless of the plugin version.
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
| return false, i18n.NewError(w.ctx, i18n.MsgWSClosing) | ||
| } | ||
|
|
||
| retry = !initial || attempt < w.initialRetryAttempts |
There was a problem hiding this comment.
Want to highlight since it's not obvious in the diff - the logic around initialRetryAttempts appears to have been broken in the original code (so that it would only try once to connect on startup before failing). It's tangential to the bulk of the changes in this PR, but it should be working now (with unit test added to demonstrate).
awrichar
changed the title
Only requests that may rely on an updated list of peers are blocked. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
3 participants
When enabled, FireFly will post to
/api/v1/initon the DX connector with a list of all current nodes,before initial connect of the websocket and before each reconnect.