Query service: fix logging errors on SIGINT #1601
Conversation
jan25
requested review from
black-adder,
jpkrohling,
objectiser,
pavolloffay,
tiffon,
vprithvi and
yurishkuro
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1601 +/- ##
==========================================
- Coverage 98.81% 98.76% -0.05%
==========================================
Files 191 191
Lines 9162 9165 +3
==========================================
- Hits 9053 9052 -1
- Misses 85 88 +3
- Partials 24 25 +1
Continue to review full report at Codecov.
|
cmd/flags/admin.go
Outdated
| @@ -113,7 +113,8 @@ func (s *AdminServer) serveWithListener(l net.Listener) { | |||
| s.server = &http.Server{Handler: recoveryHandler(s.mux)} | |||
| s.logger.Info("Starting admin HTTP server", zap.Int("http-port", s.adminPort)) | |||
| go func() { | |||
| if err := s.server.Serve(l); err != nil { | |||
| err := s.server.Serve(l) | |||
| if err != nil && err != http.ErrServerClosed { | |||
There was a problem hiding this comment.
should we make it consistent with the other and use case nil, http.ErrServerClosed, cmux.ErrListenerClosed:?
jan25
force-pushed
the
actually-fix-query-error
branch
from
7eb069e to
3b6a3d2
Compare
|
As a side note: When i just clicked "Merge master" in this PR the merge commit is not signed automatically, perhaps something to fix in the github workflow? |
|
it's the |
jan25
force-pushed
the
actually-fix-query-error
branch
from
7ec8dbc to
e9ac256
Compare
|
Added test to check for errors in logs on I may have messed up git history when trying to sign one of my commits i pushed earlier. Now DCO complains about author issue on a commit which came from merging master |
| @@ -114,6 +119,7 @@ func (s *Server) Start() error { | |||
| // Start GRPC server concurrently | |||
| go func() { | |||
| s.svc.Logger.Info("Starting GRPC server", zap.Int("port", s.queryOptions.Port)) | |||
|
|
|||
| if err := s.grpcServer.Serve(grpcListener); err != nil { | |||
There was a problem hiding this comment.
this is the same pattern as with the other two servers, but it works fine?
There was a problem hiding this comment.
Yes, grpc doesn't complain. I guess it has to do with how grpc.Serve() is implemented. Unlike with http.Serve(), grpc doesn't throw errors after grpc.Stop()
|
you can try squashing the commits to have a single signed commit, or alter the commit message in the old commit via interactive rebase. |
Signed-off-by: Abhilash Gnan <abhilashgnan@gmail.com>
jan25
force-pushed
the
actually-fix-query-error
branch
from
3d4be90 to
f0c01eb
Compare
|
@jan25 did you try to increase coverage? |
|
Thanks for the review! The additional test case tried to improve the coverage. I think codecov is talking about the |
Signed-off-by: Abhilash Gnan abhilashgnan@gmail.com
Which problem is this PR solving?
Short description of the changes
Close()on them). This PR handles such errors correctly and stops from loggingPreviously merged PR: #1598 - I think, fixed the similar issue, but solving for SIGTERM. So it stopped from logging errors when i tested rebuilt query image inside a all-in-one pod