Skip to content
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

[core] Add basic implementation to support REST Catalog #4553

Merged
merged 60 commits into from
Nov 28, 2024

Conversation

jerry-024
Copy link
Contributor

@jerry-024 jerry-024 commented Nov 20, 2024

Purpose

Add basic implementation to support REST Catalog

Linked issue: #4540

Add basic implementation to support REST Catalog

Tests

UT:

  • DefaultErrorHandlerTest
  • HttpClientTest
  • RESTCatalogTest
  • RESTObjectMapperTest

API and Format

No

Documentation

@jerry-024 jerry-024 marked this pull request as draft November 20, 2024 06:00
@jerry-024 jerry-024 changed the title [core] Add basic class to support REST Catalog [core] Add basic implementation to support REST Catalog Nov 20, 2024

/** Request to get config. */
@JsonIgnoreProperties(ignoreUnknown = true)
public class ConfigRequest implements RESTRequest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it contains warehouse? Or maybe we cannot let user to config warehouse option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter should disable the warehouse because catalogs and warehouses are one-to-one for defined catalogs, so the client should not be able to define a warehouse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As get config use HTTP get is enough, i deleted this class.


private static OkHttpClient createHttpClient(HttpClientOptions httpClientOptions) {
BlockingQueue<Runnable> workQueue =
new LinkedBlockingQueue<>(httpClientOptions.queueSize());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use SynchronousQueue, remove queueSize option.

public void close() throws Exception {}

@VisibleForTesting
Map<String, String> optionsInner(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optionsInner -> fetchOptionsFromServer

Map<String, String> initHeaders =
RESTUtil.merge(configHeaders(options.toMap()), authHeaders(token));
this.options = optionsInner(initHeaders, options.toMap());
this.baseHeader = configHeaders(this.options());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to merge authHeader too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When set up, we need to merge the auth header. Later, we will support using header() to refresh the token.

return response.merge(clientProperties);
}

private Map<String, String> authHeaders(String token) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it doesn't to be a method, just inline it.


@Override
public Catalog create(CatalogContext context) {
if (context.options().getOptional(CatalogOptions.WAREHOUSE).isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe check this in RestCatalog class?

<filters>
<filter>
<artifact>*:*</artifact>
<excludes>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why need to exclude NOTICE and LICENSE?

Copy link
Contributor

@JingsongLi JingsongLi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@JingsongLi JingsongLi merged commit 0fe18e8 into apache:master Nov 28, 2024
12 checks passed
@jerry-024 jerry-024 deleted the rest-catalog branch December 2, 2024 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants