[#4259] feat(core): support custom rest api#4261
Conversation
|
@yuqi1129 @jerryshao Could you please review this when you have time? Thank you! |
conf/gravitino.conf.template
Outdated
| # The response header size of the built-in web server | ||
| gravitino.server.webserver.responseHeaderSize = 131072 | ||
| # Comma separated list of REST API packages to scan | ||
| gravitino.restApiPackages = org.apache.gravitino.server.web.rest |
There was a problem hiding this comment.
Will this affect the iceberg rest server, did you test it, @FANNG1 please also verify it.
There was a problem hiding this comment.
Besides, I think this should not be a configuration for users. "org.apache.gravitino.server.web.rest" should always be configured, right? What is the behavior if users configure others?
You'd better configure like "additionalPackages" to support additional rest packages, and make Gravitino's package as a always loaded package.
There was a problem hiding this comment.
If we use configure like "additionalPackages" that means we don't allow user to overwrite the existing REST APIs?
I'm now set org.apache.gravitino.server.web.rest as a default value and allow user to overwrite it
There was a problem hiding this comment.
Do you have such requirements to overwrite? I don't think people have such requirements.
There was a problem hiding this comment.
OK, maybe we can make it additionalPackages and support overwrite when necessary
| * @return The EventBus instance. | ||
| */ | ||
| public EventBus eventBus() { | ||
| Preconditions.checkNotNull(eventBus, "GravitinoEnv is not initialized."); |
There was a problem hiding this comment.
I remembered that for Iceberg rest service, some modules will not enabled, so it will be null, can you please confirm this with @FANNG1 , also use checkArgument not checkNotNull.
There was a problem hiding this comment.
I think this doesn't seem to conflict with the fact that if the variable is not initialized it is not allowed to get it.
If the Iceberg REST service disables the modules, it does not need to call the method.
There was a problem hiding this comment.
If the Iceberg REST service disables the modules, it does not need to call the method.
+1
|
|
||
| public static final ConfigEntry<Optional<String>> REST_API_EXTENSION_PACKAGES = | ||
| new ConfigBuilder("gravitino.extension.restApiPackages") | ||
| public static final ConfigEntry<Optional<List<String>>> REST_API_EXTENSION_PACKAGES = |
There was a problem hiding this comment.
Do we still need Optional<List<String>>? I think we can use an empty list as a default value?
|
LGTM，except minor comments |
docs/gravitino-server-config.md
Outdated
| | `gravitino.server.webserver.responseHeaderSize` | Maximum size of HTTP responses. | `131072` | No | 0.1.0 | | ||
| | `gravitino.server.shutdown.timeout` | Time in milliseconds to gracefully shut down of the Gravitino webserver. | `3000` | No | 0.2.0 | | ||
| | `gravitino.server.webserver.customFilters` | Comma-separated list of filter class names to apply to the API. | (none) | No | 0.4.0 | | ||
| | `gravitino.extension.restApiPackages` | Comma-separated list of REST API packages to expand | (none) | No | 0.6.0 | |
There was a problem hiding this comment.
docs should also be updated.
|
CI failure seems unrelated to the changes, do you have any other comments? @jerryshao @FANNG1 |
|
I don't have further comments, just waiting for CI to pass. |
3 participants
What changes were proposed in this pull request?
fromStringforCatalog.TypeandNamespaceGravitinoEnvextensionalGravitinoEnvWhy are the changes needed?
Fix: #4259
Does this PR introduce any user-facing change?
yes, users now can use custom REST API packages
How was this patch tested?
test locally