Opensearch connection related changes for #1985,2264. #2716
Conversation
Signed-off-by: mallikagogoi7 <[email protected]>
mallikagogoi7
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
kkondaka,
cmanning09,
asifsmohammed,
dlvenable and
oeyh
as code owners
|
|
||
| package org.opensearch.dataprepper.plugins.source.opensearch; | ||
|
|
||
| public class SourceInfo { |
There was a problem hiding this comment.
From an initial look at this PR, let's rename to ClusterInfo. I think this represent more of what this object is trying to capture. Source has separate meaning in DataPrepper and I believe it makes sense to make this more related to OpenSearch/Elasticsearch.
| this.osVersion = osVersion; | ||
| } | ||
|
|
||
| public Boolean getHealthStatus() { |
There was a problem hiding this comment.
Health for a cluster can only be red, yellow, & green. Let's update this to reflect that status type.
| * @throws ParseException | ||
| * This method will check health of the source. if green or yellow and then it can be used for further processing | ||
| */ | ||
| public SourceInfo checkStatus(final OpenSearchSourceConfiguration openSearchSourceConfiguration, final SourceInfo sourceInfo) throws IOException, ParseException { |
There was a problem hiding this comment.
This method name does not make align with the return object. I would recommend renaming to some thing that aligns with the function of the method. Perhaps: getClusterInfo
| private String osVersion; | ||
|
|
||
| private String dataSource; |
There was a problem hiding this comment.
Let's make these two fields final. They should not change through the lifecycle of these cluster.
| * @return | ||
| * This method will help to identify the source information eg(opensearch,elasticsearch) | ||
| */ | ||
| public String getSourceInfo(final OpenSearchSourceConfiguration openSearchSourceConfiguration) { |
There was a problem hiding this comment.
Do we need this method? I would think we would need one method in this provider to build a ClusterInfo object. The object should be re-usable.
| this.osVersion = osVersion; | ||
| } | ||
|
|
||
| public Boolean getHealthStatus() { |
There was a problem hiding this comment.
Additionally, Should this Cluster object make API's calls to actually fetch the Health of the cluster instead of relying on the provider to handle this?.
@mallikagogoi7 would you mine putting out a skeleton / boilerplate PR for this work? I think we can collaborate on class design here to improve the code structure. Thanks!
| import java.util.List; | ||
|
|
||
|
|
||
| public class PrepareConnection { |
There was a problem hiding this comment.
Do we need a separate class for this that needs to be instantiated? I think we possibly use static method here to create clients.
| startProcess(openSearchSourceConfiguration,buffer); | ||
| } | ||
|
|
||
| private void startProcess(final OpenSearchSourceConfiguration openSearchSourceConfiguration,final Buffer<Record<Event>> buffer) { |
There was a problem hiding this comment.
I would recommend pulling out the business logic in this class into a separate service. That way this main class can construct all the required classes and execution can happen separately. This will help with unit testing down the road as well as we can inject mocks for classes which create connections to external resources. Let me know if you have questions on this.
|
Closing this PR and creating new PR as per the feedback received related to package structure and we have raised a new PR with boiler plate code 2734 |
Description
Opensearch connection related changes for #1985,2264.
Issues Resolved
Resolved #1985
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.