Dead code in ES Sink

[Rex Fenley]

Hi,

I was looking through ES options trying to diagnose a SocketTimeOut we're receiving on the ES TableAPI connector. A bug report on elasticsearch's github[1] indicated I might want to set max retry timeout really high, but from what I can tell it's not even consumed anywhere [2].

[1] https://github.com/elastic/elasticsearch/issues/33342

[2] https://github.com/apache/flink/blob/97bfd049951f8d52a2e0aed14265074c4255ead0/flink-connectors/flink-connector-elasticsearch-base/src/main/java/org/apache/flink/streaming/connectors/elasticsearch/table/ElasticsearchOptions.java#L131

Should [2] be removed?

--

[Aljoscha Krettek]

On 2021/01/12 15:04, Rex Fenley wrote:
>[2]
>https://github.com/apache/flink/blob/97bfd049951f8d52a2e0aed14265074c4255ead0/flink-connectors/flink-connector-elasticsearch-base/src/main/java/org/apache/flink/streaming/connectors/elasticsearch/table/ElasticsearchOptions.java#L131
>
>Should [2] be removed?

The link seems to not work anymore but I'm guessing you're referring to
`CONNECTION_MAX_RETRY_TIMEOUT_OPTION`. This is used in the
`*DynamicSinkFactory` classes, such as [1]. These can be used when
defining Table API/SQL sources using DDL or the programmatic API. The
actual field is never used but it will be used to check the allowed
options when verifying what users specify via "string" options.

[1]
https://github.com/apache/flink/blob/ee653778689023ddfdf007d5bde1daad8fbbc081/flink-connectors/flink-connector-elasticsearch7/src/main/java/org/apache/flink/streaming/connectors/elasticsearch/table/Elasticsearch7DynamicSinkFactory.java#L98

[Rex Fenley]

>The actual field is never used but it will be used to check the allowed options when verifying what users specify via "string" options.

Are you saying that this option does get passed along to Elasticsearch still or that it's just arbitrarily validated? According to [1] it's been deprecated in ES 6 and removed in ES 7.

[1] https://github.com/elastic/elasticsearch/pull/38085

On Wed, Jan 13, 2021 at 12:50 AM Aljoscha Krettek <[hidden email]> wrote:

On 2021/01/12 15:04, Rex Fenley wrote:
>[2]
>https://github.com/apache/flink/blob/97bfd049951f8d52a2e0aed14265074c4255ead0/flink-connectors/flink-connector-elasticsearch-base/src/main/java/org/apache/flink/streaming/connectors/elasticsearch/table/ElasticsearchOptions.java#L131
>
>Should [2] be removed?

The link seems to not work anymore but I'm guessing you're referring to
`CONNECTION_MAX_RETRY_TIMEOUT_OPTION`. This is used in the
`*DynamicSinkFactory` classes, such as [1]. These can be used when
defining Table API/SQL sources using DDL or the programmatic API. The
actual field is never used but it will be used to check the allowed
options when verifying what users specify via "string" options.

[1]
https://github.com/apache/flink/blob/ee653778689023ddfdf007d5bde1daad8fbbc081/flink-connectors/flink-connector-elasticsearch7/src/main/java/org/apache/flink/streaming/connectors/elasticsearch/table/Elasticsearch7DynamicSinkFactory.java#L98

--

[Aljoscha Krettek]

On 2021/01/13 07:50, Rex Fenley wrote:
>Are you saying that this option does get passed along to Elasticsearch
>still or that it's just arbitrarily validated? According to [1] it's been
>deprecated in ES 6 and removed in ES 7.
>
>[1] https://github.com/elastic/elasticsearch/pull/38085

Sorry, I wasn't being very clear. I meant that we just pass it on to ES.  
In light of it being deprecated, I think it makes sense to keep it as
long as we support ES 6. What do you think?

Side note: we still have an ES 5 connector... 😅 There was a discussion
about dropping it, but it wasn't conclusive. [1]

[1]
https://lists.apache.org/thread.html/rb957e7d7d5fb9bbe25e5fbc56662749ee1bc551d36e26c58644f60d4%40%3Cdev.flink.apache.org%3E

Best,
Aljoscha