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.

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

Fix HTTPStatusException propagation #1109

Merged
merged 2 commits into from
Aug 10, 2020
Merged

Conversation

AndrejMitrovic
Copy link
Contributor

No description provided.

@AndrejMitrovic AndrejMitrovic added the type-bug Things don't work as they were intended to, or the way they were intended to work doesn't make sense label Aug 10, 2020
@AndrejMitrovic
Copy link
Contributor Author

Well I really thought this would fix it, but apparently there are more hidden bugs. Sigh.

@AndrejMitrovic
Copy link
Contributor Author

Ah maybe it also requires Jay's PR.

@AndrejMitrovic
Copy link
Contributor Author

Ah maybe it also requires Jay's PR.

The reason I say this is because there are still no logs like this: https://github.com/bpfkorea/agora/blob/b602254d522f5bc2e5b5e96525c4d61e7c9180ff/source/agora/network/NetworkManager.d#L186

We need to handle vibe.d exceptions properly,
if an HTTPStatusException is thrown we need to
propagate it to the calling code so it can
be properly handled.
@AndrejMitrovic
Copy link
Contributor Author

Ah maybe it also requires Jay's PR.

Nope it didn't work with the combination of that.

So my PR fixes one bug but it doesn't solve the underlying problem.

@AndrejMitrovic AndrejMitrovic changed the title Fix CircleCI failures Fix HTTPStatusException propagation Aug 10, 2020
It might resolve the CircleCI failures, but it would
only treat the symptom and not solve the underlying problem.
@codecov
Copy link

codecov bot commented Aug 10, 2020

Codecov Report

Merging #1109 into v0.x.x will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x    #1109      +/-   ##
==========================================
+ Coverage   78.58%   78.62%   +0.03%     
==========================================
  Files          83       84       +1     
  Lines        7912     7953      +41     
==========================================
+ Hits         6218     6253      +35     
- Misses       1694     1700       +6     
Flag Coverage Δ
#integration 39.24% <100.00%> (-0.32%) ⬇️
#unittests 77.34% <50.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
source/agora/network/NetworkClient.d 97.22% <100.00%> (+0.16%) ⬆️
source/agora/utils/Log.d 68.51% <0.00%> (-3.71%) ⬇️
source/agora/consensus/protocol/Nominator.d 90.62% <0.00%> (-1.05%) ⬇️
source/agora/consensus/validation/Block.d 97.91% <0.00%> (-0.51%) ⬇️
source/agora/utils/Test.d 96.58% <0.00%> (ø)
source/agora/node/Ledger.d 97.04% <0.00%> (ø)
source/agora/test/NetworkManager.d 96.96% <0.00%> (ø)
source/agora/consensus/data/Block.d 100.00% <0.00%> (ø)
source/agora/test/EnrollmentManager.d 96.55% <0.00%> (ø)
source/agora/test/VariableBlockSize.d 90.47% <0.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e25f0e...a8e7c3f. Read the comment docs.

@AndrejMitrovic
Copy link
Contributor Author

With the second commit the log files finally show connecting to the FullNode:

node-3_1_5841cdea84e9 | 2020-08-10 08:51:32,299 Info [agora.network.NetworkManager] - Found new FullNode: http://node-0:1826

But this is just treating the symptom and not fixing the underlying problem. However maybe we can at least be unblocked..

@Geod24 Geod24 merged commit 2b029fc into bosagora:v0.x.x Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug Things don't work as they were intended to, or the way they were intended to work doesn't make sense
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants