kafka-multi-cluster #21
Conversation
|
| var clientOptions = new ClientOptions | ||
| { | ||
| TIKA_ENABLE_MULTI_CLUSTER = false, | ||
| TIKA_API_ENDPOINT = "http://localhost:3000", |
There was a problem hiding this comment.
Can we change this port to 3001? It will collide a million billion times with the backstage frontend client
| TIKA_ENABLE_MULTI_CLUSTER = ConfToBool(conf, "TIKA_ENABLE_MULTI_CLUSTER", false); | ||
|
|
||
| // Multi-cluster config | ||
| TIKA_MULTI_CLUSTER_HOSTNAME_PREFIX = ConfToString(conf, "TIKA_MULTI_CLUSTER_HOSTNAME_PREFIX", "http://tika-"); |
There was a problem hiding this comment.
Use options pattern for configs (IOptions) and ensure sensible defaults are set at the "property level"
| @@ -6,19 +6,28 @@ namespace Tika.RestClient.Factories | |||
| { | |||
| public static class RestClientFactory | |||
| { | |||
| public static IRestClient Create(HttpClient httpClient) | |||
| public static IRestClient Create(HttpClient httpClient, ClientOptions options, string cluster = "") | |||
There was a problem hiding this comment.
Default values in .net core should be assigned using default keyword so we dont have to change the right hand side of the assignment if we change the type on the left hand side!
| } | ||
| else | ||
| { | ||
| httpClient.BaseAddress = new Uri($"{options.Value?.TIKA_MULTI_CLUSTER_HOSTNAME_PREFIX}-{cluster}:3000"); |
There was a problem hiding this comment.
Port is hardcoded, is that not a problem if sensible defaults is hardcoded in multiple places
| { | ||
| var httpResponseMessage = await _httpClient.GetAsync( | ||
| new Uri(ACLS_ROUTE, UriKind.Relative) | ||
| new Uri(Utilities.MakeUrl(_clientOptions, ACLS_ROUTE, clusterId), UriKind.Absolute) |
There was a problem hiding this comment.
Maybe call this a helper? and not a utility since its a very specific function that solves a very specific problem?
| Task<IEnumerable<ApiKey>> GetAllAsync(); | ||
| Task<ApiKey> CreateAsync(ApiKeyCreate apiKeyCreate); | ||
| Task DeleteAsync(string key); | ||
| Task<IEnumerable<ApiKey>> GetAllAsync(string clusterId = null); |
There was a problem hiding this comment.
default value for "default" keyword is "null", "" or 0, false, etc. Please assign to = default and not = null
| { | ||
| if (clientOptions.TIKA_ENABLE_MULTI_CLUSTER) | ||
| { | ||
| return $"{clientOptions.TIKA_MULTI_CLUSTER_HOSTNAME_PREFIX}{cluster}:3000{path}"; |
DON'T MERGE.