fix: efficient error handling in manager-api including graceful shutdown, self contained methods. #1814
Conversation
|
Deploy preview for apisix-dashboard ready! Built with commit 84e18e6 |
|
@nic-chen still same issue. any idea why? this time I pull the master, created a new branch and handpicked the new lines (as the number of lines were less) of commit instead of cherry-picking to avoid this CI invocation, but still the same problem |
|
Hi, I just sent an email to GitHub Support, to ask for their help about this case. |
|
Nice🔥 |
|
I can't think of why. This should be the same as other new PRs, but other PRs have no such problem. Just wait for the reply from GitHub Support.. |
|
Hii, any update :) |
|
hi @bisakhmondal please sync code from branch master |
|
Thanks!! Done. |
|
Ou Nice!! It seems no stalling CI from node build. Thank you both :) |
Codecov Report
@@ Coverage Diff @@
## master #1814 +/- ##
==========================================
- Coverage 71.45% 61.85% -9.60%
==========================================
Files 172 47 -125
Lines 6121 3183 -2938
Branches 711 0 -711
==========================================
- Hits 4374 1969 -2405
+ Misses 1498 898 -600
- Partials 249 316 +67
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
api/cmd/managerapi.go
Outdated
| @@ -65,6 +66,14 @@ func printVersion() { | |||
|
|
|||
| // NewManagerAPICommand creates the manager-api command. | |||
| func NewManagerAPICommand() *cobra.Command { | |||
| cobra.OnInitialize(func() { | |||
| var err error | |||
| service, err = createService() | |||
There was a problem hiding this comment.
Use short variables creation.
There was a problem hiding this comment.
Hi @tokers, I'm afraid that's not possible here due to variable shadowing.
service is a global variable and assignment through go's short assignment operator (:=) will create a brand new service var in this scope without referencing the global one.
I looked into the docs and also cross-checked ref
An identifier declared in a block may be redeclared in an inner block. While the identifier of the inner declaration is in scope, it denotes the entity declared by the inner declaration.
Anyway, Thank you. Got the scope to re-read the docs :)
There was a problem hiding this comment.
Wait, what!! that's not from this PR 🤔
bisakhmondal
force-pushed
the
fix-store-init-error
branch
from
b9aea51 to
c66c914
Compare
bisakhmondal
changed the title
|
Any idea why the upstream test suite is breaking🤔, the PR doesn't touch anything on that segment and moreover, it's HTTP 400!! All starts with But it's working fine locally |
|
Hi @bisakhmondal, there have some errors in Backend CI. |
Yeshh!! Sad moments🥺 |
| @@ -26,6 +26,9 @@ import ( | |||
|
|
|||
| // WritePID write pid to the given file path. | |||
| func WritePID(filepath string) error { | |||
| if _, err := os.Stat(filepath); err == nil { | |||
| return fmt.Errorf("instance of Manager API already running: a pid file exists in %s", filepath) | |||
There was a problem hiding this comment.
Does this mean the manager can't start successfully if there exists a PID file?
There was a problem hiding this comment.
We may add an option to start forcely.
There was a problem hiding this comment.
Hii guys, as we are now supporting manager-api as an os service, the PID file should not be overwritten unknowingly, because then ./manager-api stop won't work. we have to take help from the os service manager explicitly.
We may add an option to start forcely.
Yes, we can do that.
|
Hi guys, the CI hadn't passed in this PR. Some checking was left to be done from my side. I truly am sorry, I got caught up in university exams for a few days. I think we should revert the commit. CI has failed in the master too. #1814 (comment) Sorry for the inconvenience. I am updating the PR as soon as possible. Thank you. |
|
@bisakhmondal Hi, if it's ok, could you please send a new PR to fix that failure? |
Ya, that sounds awesome. but yet, I can't reproduce the failure(totally irrelevant to this PR) locally even with the same environment like CI inside a docker container, |
@juzhiyuan It seems to be a CI glitch, now the master is fine after a recent commit. Even the last commit has passed all CI. What do you think, can we ignore? |
|
I have to say it's unstable, please create an issue to track this :) |
Please answer these questions before submitting a pull request, or your PR will get closed.
Why submit this pull request?
What changes will this PR take into?
1. Graceful handling of resources.
2. Self contained Manager API.
Fatalfis not graceful inside goroutines, which leads to an abrupt termination.3. Deligating error handling to existing
spf13/cobrapackage.4. Removing unnecessary proliferation of golang
panics. Why not use the user friendly interface already provided by cobra. To be honest, output ofpaniclooks scary 😂Closing of all opened resource has been put into deferred execution. So incase any panic occurs during runtime, the respective closers of the resource gets executed safely. Till now, for panics, the main goroutine was getting closed ungracefully.
Detailed error description if etcd contains corrupted data and exit with nonzero error code.
Related issues
reopening #1705
closes #1684
Checklist: