-
Notifications
You must be signed in to change notification settings - Fork 73
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
feat: Karapace modernization with uvicorn, FastAPI and OpenTelemetry #1039
Conversation
- we create a standalone module for SR related components - we use DI to wire together the SR dependencies - we move the routers to own folder - we move the config initialization to DI and app startup
The config for backup tool is still the full Karapace config. Config for Karapace is a set of environment variables opposed to prior JSON file. Backup tool will still use the JSON format.
- we drop the `Dockerfile.dev`
- the `schema_tool` runs against avro schemas at a specific path
The REST Proxy requires still split from karapace.core modules, e.g. configuration class is there.
3c28958
to
34f13d5
Compare
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.
Very nice 👍. Just saw some comments in code that maybe can be removed as well as one question for the authorizer.
karapace_container = providers.Container(KarapaceContainer) | ||
no_auth_authorizer = providers.Singleton(NoAuthAndAuthz) | ||
http_authorizer = providers.Singleton(HTTPAuthorizer, auth_file=karapace_container.config().registry_authfile) | ||
# http_authorizer = providers.Singleton(HTTPAuthorizer, create_http_authorizer) |
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.
should commented out stuff be removed?
set_health_status_tracing_attributes(health_check_span=health_check_span, health_status=health_status) | ||
|
||
# if self._auth is not None: | ||
# resp["schema_registry_authfile_timestamp"] = self._auth.authfile_last_modified |
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.
comment ☝️
telemetry_resource: Resource = Provide[TelemetryContainer.telemetry_resource], | ||
) -> None: | ||
LOG.info("Setting OTel meter provider") | ||
# metrics.set_meter_provider(MeterProvider(resource=telemetry_resource, metric_readers=[meter.get_metric_reader()])) |
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.
comment ☝️
controller: KarapaceSchemaRegistryController = Depends(Provide[SchemaRegistryContainer.schema_registry_controller]), | ||
) -> CompatibilityLevelResponse: | ||
subject = Subject(unquote_plus(subject)) | ||
if authorizer and not authorizer.check_authorization(user, Operation.Read, f"Subject:{subject}"): |
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.
why do we have to check for authorizer here? Shouldn't this be either instance of HTTPAuthorizer or NoAuthAndAuthz?
) | ||
|
||
|
||
# TODO is this needed? Is this actually the ids/schema/id/schema?? |
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.
This TODO intentional?
# controller: KarapaceSchemaRegistryControllerDep, | ||
# ) -> SchemasResponse: | ||
# # TODO retrieve by id only schema | ||
# return await controller.schemas_get() |
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.
comment ☝️
# f"{new_config['advertised_protocol']}://{new_config['advertised_hostname']}:{new_config['advertised_port']}" | ||
|
||
# set tags if not set | ||
# new_config["tags"]["app"] = "Karapace" |
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.
comments ☝️
# env_name, | ||
# ) | ||
# | ||
# config[config_name] = parse_env_value(env_val) |
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.
comments ☝️
As agreed with @jjaakola-aiven, merging this one |
wow amazing! Congrats for the awesome work @jjaakola-aiven |
About this change - What it does
Karapace modernization to use uvicorn, FastAPI and OpenTelemetry.
Why this way
The replacement of
rapu
andaiohttp
is required for having more feature complete framework. The FastAPI enables the use of community plugins that are easier to integrate.The new and changed code is reorganized into directories:
src/karapace/api
- Schema registry FastAPI server and routerssrc/karapace/core
- Schema registry core enginesrc/karapace/kafka_rest_apis
- REST ProxyEntry points:
src/karapace/kafka_rest_apis/__main__.py
- REST Proxy main entry pointsrc/karapace/__main__.py
- Schema registry main entry pointImprovements to do later
rapu
andaiohttp
.src/karapace/kafka_rest_apis
.