[content-service] Refactor to use baseserver #9973
Conversation
easyCZ
changed the title
easyCZ
force-pushed
the
mp/content-service-baseserver
branch
4 times, most recently
from
445fb19 to
36dae42
Compare
easyCZ
force-pushed
the
mp/content-service-baseserver
branch
from
fdc536b to
e4a8f25
Compare
easyCZ
force-pushed
the
mp/baseserver-init-metrics
branch
from
d603efd to
73cb046
Compare
easyCZ
force-pushed
the
mp/content-service-baseserver
branch
from
7c638e0 to
bdba136
Compare
easyCZ
force-pushed
the
mp/content-service-baseserver
branch
from
2836760 to
c671645
Compare
|
From my testing on preview, this looks to be working but I'm lacking context on some flows which use the content service, so would appreciate help/direction on what needs to be validated. |
github-actions
bot
added
team: IDE
team: workspace
| Storage StorageConfig `json:"storage"` | ||
| } | ||
|
|
||
| type TLSConfig struct { |
There was a problem hiding this comment.
TLS config is now part of baseserver so using it directly.
| Prometheus: config.Prometheus{ | ||
| Addr: fmt.Sprintf(":%d", PrometheusPort), | ||
| }, | ||
| PProf: config.PProf{ |
There was a problem hiding this comment.
Wasn't used anywhere besides as a value passed into the server as config.
| Service: config.Service{ | ||
| Addr: fmt.Sprintf(":%d", RPCPort), | ||
| }, | ||
| Prometheus: config.Prometheus{ |
There was a problem hiding this comment.
There's a default metrics port for each baseserver so using that instead.
| contentService, err := service.NewContentService(cfg.Storage) | ||
| if err != nil { | ||
| log.WithError(err).Fatalf("cannot create content service") | ||
| log.WithError(err).Fatalf("Cannot create content service") |
There was a problem hiding this comment.
please leave this lowercase. The vast majority of our error messages is lowercase, and we should keep things consistent
There was a problem hiding this comment.
General practice in Go (and elsewhere) is to uppercase logs to make them sentences. They are designed to be consumed by humans and first letter uppercase is slightly easier to visually grep. Even our style guide uses first letter uppercase in logs https://github.com/uber-go/guide/blob/master/style.md#reduce-nesting
The change here is really just a drive by improvement to be more consistent with general practice.
Errors, on the other hand, should never start with an uppercase because they get wrapped and composed so don't really form sentences.
There was a problem hiding this comment.
Here's another reference from go code review style guide
There was a problem hiding this comment.
Fair point.
/hold cancel
Description
Refactors content service to use baseserver for server setup. Removes scattered config and allows us to normalize server setup across components.
Related Issue(s)
Similar to #10005
How to test
Content service runs in preview, unit tests pass
Release Notes
Documentation
NONE
/hold