reswqa commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1857562853
Thanks @schulzp, nice one.
--
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
boring-cyborg[bot] commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1857562560
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message,
reswqa merged PR #83:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/83
--
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:
reswqa commented on code in PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#discussion_r1427487497
##
flink-connector-elasticsearch-base/src/test/java/org/apache/flink/connector/elasticsearch/sink/ElasticsearchSinkBuilderBaseTest.java:
##
@@ -99,7
reswqa commented on code in PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#discussion_r1427487497
##
flink-connector-elasticsearch-base/src/test/java/org/apache/flink/connector/elasticsearch/sink/ElasticsearchSinkBuilderBaseTest.java:
##
@@ -99,7
reswqa commented on code in PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#discussion_r1427487497
##
flink-connector-elasticsearch-base/src/test/java/org/apache/flink/connector/elasticsearch/sink/ElasticsearchSinkBuilderBaseTest.java:
##
@@ -99,7
reswqa commented on code in PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#discussion_r1427487497
##
flink-connector-elasticsearch-base/src/test/java/org/apache/flink/connector/elasticsearch/sink/ElasticsearchSinkBuilderBaseTest.java:
##
@@ -99,7
schulzp commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1855334073
@reswqa, thanks `TestingSinkWriterMetricGroup` works on all tested versions.
I only faced the arch rules issue locally, so far.
--
This is an automated message from
reswqa commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1855090968
@schulzp, thank you for the investigation.
1. I think `TestingSinkWriterMetricGroup` might help.
2. Yes, this is a violations indeed. But I'm not sure if this
schulzp commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1853540370
@reswqa, there are two things that need to be fixed:
1. use `MetricsGroupTestUtils#mockWriterMetricGroup()` instead of
`InternalSinkWriterMetricGroup.mock()`,
reswqa commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1853168166
@schulzp That PR has been merged. I have pushed an empty commit to your
branch to re-trigger CI.
--
This is an automated message from the Apache Git Service.
To
reswqa commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1851249351
We should waiting for https://github.com/apache/flink/pull/23876 to be
merged(This won't take long as it has already been approved by 3 votes). After
that, CI should be
schulzp commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1850111290
@reswqa, fixed the cause of the `NotSerializableException` (implicit
reference of anonymous class to `ElasticsearchSinkBuilderBase`). CI should pass
now.
--
This is
schulzp commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1845356753
@reswqa, I overrode the missing methods required by 1.18.0's
`Sink.InitContext`.
--
This is an automated message from the Apache Git Service.
To respond to the
schulzp commented on code in PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#discussion_r1418511130
##
flink-connector-elasticsearch-base/src/test/java/org/apache/flink/connector/elasticsearch/sink/DefaultBulkResponseInspectorTest.java:
##
@@ -0,0
schulzp commented on code in PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#discussion_r1418486556
##
flink-connector-elasticsearch-base/src/test/java/org/apache/flink/connector/elasticsearch/sink/DefaultBulkResponseInspectorTest.java:
##
@@ -0,0
reswqa commented on code in PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#discussion_r1418211399
##
flink-connector-elasticsearch-base/src/test/java/org/apache/flink/connector/elasticsearch/sink/DefaultBulkResponseInspectorTest.java:
##
@@ -0,0
schulzp commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1843307919
@afedulov, fixed the test (and extended it)
@reta, I added a `FailureHandler` with a default implementation that
resembles the current fail-on-any-failure
schulzp commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1843137406
@afedulov, sure, I'll implement it with plain java.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub
afedulov commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1843121034
@schulzp thanks!
We try to avoid Mockito usage, unless it is not possible because because of
external dependencies that required concrete classes rather than
reta commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1843109798
> I assume there shared code to be reused and that is more about API
consistency?
There is no shared code (I think you meant that) and indeed, it is more
about API
schulzp commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1843099110
@reta, sure, so I'll rename my interfaces to match those from opensearch and
add the factory to the opensearch sink builder afterwards. I assume there
shared code to
reta commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1843053300
> @reta, I checked out that the opensearch failure handler. That achieves
the same in regard of error handling. However, it does not allow capturing
additional metrics.
schulzp commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1843046355
@afedulov, thanks! I added a test that makes sure the inspector is passed to
the `ElasticsearchWriter`.
@reswqa, I looked into the pipeline failed pipelines:
reta commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1842971156
@schulzp there was a similar change introduced into
`flink-connector-opensearch`, I believe we could backport it to the
Elasticsearch connector to have a similar model of
afedulov commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1841113426
@schulzp thanks for the contribution. The approach looks solid. I believe
what is missing are some tests that check that the non-default inspector is
actually
boring-cyborg[bot] commented on PR #83:
URL:
https://github.com/apache/flink-connector-elasticsearch/pull/83#issuecomment-1835980923
Thanks for opening this pull request! Please check out our contributing
guidelines. (https://flink.apache.org/contributing/how-to-contribute.html)
--
schulzp opened a new pull request, #83:
URL: https://github.com/apache/flink-connector-elasticsearch/pull/83
Extracted `BulkResponseInspector` interface to allow custom handling of
(partially) failed bulk requests. If not overridden, default behaviour remains
unchanged and partial failures
28 matches
Mail list logo
