DNS: Send ACK to Maker after successful address posting #414
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #414 +/- ##
==========================================
+ Coverage 70.06% 70.09% +0.02%
==========================================
Files 34 34
Lines 4263 4270 +7
==========================================
+ Hits 2987 2993 +6
- Misses 1276 1277 +1 ☔ View full report in Codecov by Sentry. |
|
Do these changes look good? |
|
Ended up using |
|
Do these changes look good? Also, I noticed some tests have failed. The logs are suggesting to update workflow to use |
src/maker/server.rs
Outdated
| match status_recv.recv() { | ||
| Ok(status) => { | ||
| if let Err(e @ MakerError::UnexpectedMessage { .. }) = status { | ||
| log::error!("{:?}", e); | ||
| return Err(e); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| log::error!("Failed to receive from status channel {:?}", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why are we doing this? The errors and Ack are already handled in the thread loop. Even if you have it, it will only execute once for the first attempt, and it's not doing anything extra than you have already covered in the logic above.
If I am not wrong you will have an mpsc error, as the receiver will go out of scope before you receive a response from DNS.
This is redundant, and can be removed.
There was a problem hiding this comment.
We are doing this to return a hard error as suggested in the following comment: #414 (comment)
Also, I now notice that this will work only for first attempt, upon sending another message it fails as if the receiving end of the channel is disconnected. Here:
Lines 254 to 259 in 13b79fe
There was a problem hiding this comment.
discussed with @Shourya742 , we shouldn't do hard errors for DNS Nacks, so this can be removed.
src/maker/server.rs
Outdated
| // Do not block the process thread | ||
| status_sender | ||
| .send(Err(MakerError::General( | ||
| "Maximum retry limit reached, avoiding thread blocking", | ||
| ))) | ||
| .unwrap(); |
There was a problem hiding this comment.
you are suppose to put it in the complement of the if condition, and not put inside the if condition.
src/maker/server.rs
Outdated
| // Do not block the process thread | ||
| status_sender | ||
| .send(Err(MakerError::General( | ||
| "Maximum retry limit reached, avoiding thread blocking", | ||
| ))) | ||
| .unwrap(); |
There was a problem hiding this comment.
you are suppose to put it in the complement of the if condition, and not put inside the if condition.
|
Do these changes look good? |
This PR aims to address #401. Please let me know if my approach looks good to you.