-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Data mover micro service smoke testing #8115
Data mover micro service smoke testing #8115
Conversation
Signed-off-by: Lyndon-Li <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8115 +/- ##
==========================================
+ Coverage 59.02% 59.08% +0.06%
==========================================
Files 364 364
Lines 30272 30300 +28
==========================================
+ Hits 17867 17902 +35
+ Misses 10959 10955 -4
+ Partials 1446 1443 -3 ☔ View full report in Codecov by Sentry. |
82e3b98
to
06d83ae
Compare
Signed-off-by: Lyndon-Li <[email protected]>
06d83ae
to
ed0ef67
Compare
5545b36
to
d25a908
Compare
Signed-off-by: Lyndon-Li <[email protected]>
defer func() { | ||
go r.closeDataPath(ctx, duName) | ||
}() | ||
defer r.dataPathMgr.RemoveAsyncBR(duName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't calling closeDataPath
be a better choice instead of directly calling RemoveAsyncBR
? Why are we changing this from earlier behavior ? (same question for datadownload/pvb/pvr)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is under the context of callbacks which are called by the async backup/restore thread.
So calling closeDataPath
which calls asyncBR.Close
is not literally correct --- the async backup/restore thread is a part of the asyncBR instance so asyncBR.Close
literally needs to first wait the thread and then close all the leftovers. However, this will cause a deadlock.
This is why we needed a go routine to call closeDataPath
in the callbacks before this change. However, that is not a ideal solution.
Therefore, the logics are refactored:
- We create two functions for asyncBR,
close
andClose
. The latter one calls the former one - The async backup/restore thread calls
close
by the end itself - So the callbacks never need to care about the closure of the asyncBR, they only need to take care of their own data
Close
is used by the caller to stop asyncBR outside of the async thread
Enhance some of the data mover micro service code and fix the problems found during smoke testing