[GitHub] [flink-connector-opensearch] dannycranmer commented on pull request #5: [FLINK-30488] OpenSearch implementation of Async Sink

["dannycranmer (via GitHub)" <gi...@apache.org>]

dannycranmer commented on PR #5:
URL: https://github.com/apache/flink-connector-opensearch/pull/5#issuecomment-1488320165

   @reta I am reluctant to introduce a new Sink API based on the internal implementation unless there is a really good/semantic reason. I would prefer to encapsulate the internals via a single Flink layer that can support either `RestHighLevelClient`/`BulkProcessor` based on configuration. How will this look for SQL? We usually use a simple identifier like "opensearch", I fear that "opensearch-async" adds no semantic value to the user.
   
   We should keep the Sink API as simple as possible with sensible defaults, and allow advanced users to configure as they wish. For instance, a user should not need decide to use `OpensearchSink` vs `OpensearchAsyncSink`, they should just use `OpensearchSink` and configure as needed.
   
   There could be reasons to have 2x Sinks if they support fundamentally different features/APIs but I would expect the naming to reflect this, for example `OpensearchRestHighLevelClientSink`/`OpensearchBulkProcessorSink`. 
   
   Apologies for raising these concerns late in the process but I cannot see this has been considered before. @MartijnVisser what are your thoughts?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org