You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "lilyevsky (via GitHub)" <gi...@apache.org> on 2023/02/14 02:35:44 UTC
[GitHub] [flink-connector-opensearch] lilyevsky opened a new pull request, #9: Dependabot/maven/flink connector opensearch/org.opensearch opensearch 2.5.0
lilyevsky opened a new pull request, #9:
URL: https://github.com/apache/flink-connector-opensearch/pull/9
See comments on https://github.com/apache/flink-connector-opensearch/pull/8 .
These are the same changes re-targeted to main branch.
--
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
[GitHub] [flink-connector-opensearch] reta commented on pull request #9: Add FailureHandler
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on PR #9:
URL: https://github.com/apache/flink-connector-opensearch/pull/9#issuecomment-1429031667
> @reta The validator did not like the PR title. Not sure what it wants. Could you please fix it?
@lilyevsky I would love to but I don't have enough permissions to do that. Could you modify your title please to be: `
[FLINK-30998] Add optional exception handler to flink-connector-opensearch`?
--
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
[GitHub] [flink-connector-opensearch] lilyevsky closed pull request #9: [FLINK-30998] Add optional exception handler to flink-connector-opensearch
Posted by "lilyevsky (via GitHub)" <gi...@apache.org>.
lilyevsky closed pull request #9: [FLINK-30998] Add optional exception handler to flink-connector-opensearch
URL: https://github.com/apache/flink-connector-opensearch/pull/9
--
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
[GitHub] [flink-connector-opensearch] reta commented on a diff in pull request #9: [FLINK-30998] Add optional exception handler to flink-connector-opensearch
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #9:
URL: https://github.com/apache/flink-connector-opensearch/pull/9#discussion_r1107175148
##########
flink-connector-opensearch/pom.xml:
##########
@@ -36,7 +36,7 @@ under the License.
<!-- Allow users to pass custom connector versions -->
<properties>
- <opensearch.version>1.3.0</opensearch.version>
+ <opensearch.version>2.5.0</opensearch.version>
Review Comment:
Please revert, Opensearch 2.x needs JDK-11
--
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
[GitHub] [flink-connector-opensearch] lilyevsky commented on pull request #9: [FLINK-30998] Add optional exception handler to flink-connector-opensearch
Posted by "lilyevsky (via GitHub)" <gi...@apache.org>.
lilyevsky commented on PR #9:
URL: https://github.com/apache/flink-connector-opensearch/pull/9#issuecomment-1432145314
@reta It looks like it was wrong idea for me to make changes on ....2.5.0 branch and then create a PR targeted to main.
Let me do it all over again, it would be simpler. I will make changes in my repo in main branch and create a PR to main.
--
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
[GitHub] [flink-connector-opensearch] lilyevsky commented on pull request #9: Add FailureHandler
Posted by "lilyevsky (via GitHub)" <gi...@apache.org>.
lilyevsky commented on PR #9:
URL: https://github.com/apache/flink-connector-opensearch/pull/9#issuecomment-1429027066
@reta The validator did not like the PR title. Not sure what it wants. Could you please fix it?
--
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
[GitHub] [flink-connector-opensearch] reta commented on a diff in pull request #9: [FLINK-30998] Add optional exception handler to flink-connector-opensearch
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #9:
URL: https://github.com/apache/flink-connector-opensearch/pull/9#discussion_r1107175537
##########
flink-connector-opensearch/src/main/java/org/apache/flink/connector/opensearch/sink/OpensearchSink.java:
##########
@@ -59,19 +59,22 @@
private final BulkProcessorConfig buildBulkProcessorConfig;
private final NetworkClientConfig networkClientConfig;
private final DeliveryGuarantee deliveryGuarantee;
+ private FailureHandler failureHandler;
Review Comment:
`final` please
--
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
[GitHub] [flink-connector-opensearch] reta commented on a diff in pull request #9: [FLINK-30998] Add optional exception handler to flink-connector-opensearch
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #9:
URL: https://github.com/apache/flink-connector-opensearch/pull/9#discussion_r1107176163
##########
flink-connector-opensearch/src/main/java/org/apache/flink/connector/opensearch/sink/FailureHandler.java:
##########
@@ -0,0 +1,26 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.connector.opensearch.sink;
+
+import java.io.Serializable;
+
+/** Handler to process failures. */
+public interface FailureHandler extends Serializable {
Review Comment:
Could you please add @FunctionalInterface to this interface (for usability perspective).
##########
flink-connector-opensearch/src/main/java/org/apache/flink/connector/opensearch/sink/OpensearchSinkBuilder.java:
##########
@@ -285,6 +286,18 @@ public OpensearchSinkBuilder<IN> setAllowInsecure(boolean allowInsecure) {
return self();
}
+ /**
+ * Allows to set custom failure handler. If not set, or set to null, then the default behavior
+ * is preserved: throwing a runtime exception upon receiving a failure.
+ *
+ * @param failureHandler
+ * @return
+ */
+ public OpensearchSinkBuilder<IN> setFailureHandler(FailureHandler failureHandler) {
+ this.failureHandler = failureHandler;
Review Comment:
Please `checkNotNull(failureHandler);`
--
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
[GitHub] [flink-connector-opensearch] reta commented on a diff in pull request #9: [FLINK-30998] Add optional exception handler to flink-connector-opensearch
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #9:
URL: https://github.com/apache/flink-connector-opensearch/pull/9#discussion_r1107177071
##########
flink-connector-opensearch/src/test/java/org/apache/flink/connector/opensearch/sink/OpensearchWriterITCase.java:
##########
@@ -266,7 +266,8 @@ private OpensearchWriter<Tuple2<Integer, String>> createWriter(
null,
true),
metricGroup,
- new TestMailbox());
+ new TestMailbox(),
+ null);
Review Comment:
`DEFAULT_FAILURE_HANDLER` would help here to eliminate `null`
##########
flink-connector-opensearch/src/test/java/org/apache/flink/streaming/connectors/opensearch/OpensearchSinkTest.java:
##########
@@ -207,7 +205,7 @@ public void testItemFailureRethrownOnCheckpointAfterFlush() throws Throwable {
1,
OpType.INDEX,
new IndexResponse(
- new ShardId("test", "-", 0), "_doc", "1", 0, 0, 1, true))));
+ new ShardId("test", "-", 0), "1", 0, 0, 1, true))));
Review Comment:
Please revert
--
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
[GitHub] [flink-connector-opensearch] reta commented on a diff in pull request #9: [FLINK-30998] Add optional exception handler to flink-connector-opensearch
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #9:
URL: https://github.com/apache/flink-connector-opensearch/pull/9#discussion_r1107176742
##########
flink-connector-opensearch/src/main/java/org/apache/flink/connector/opensearch/sink/OpensearchWriter.java:
##########
@@ -343,7 +346,11 @@ private void extractFailures(BulkRequest request, BulkResponse response) {
if (chainedFailures == null) {
return;
}
- throw new FlinkRuntimeException(chainedFailures);
+ if (failureHandler == null) {
Review Comment:
Dealing with`null` is better to be avoided: we could use the default `failureHandler`:
```java
private static final FailureHandler DEFAULT_FAILURE_HANDLER = ex -> throw new FlinkRuntimeException(ex);
```
and never deal with nullability / `if`s
--
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
[GitHub] [flink-connector-opensearch] lilyevsky commented on pull request #9: [FLINK-30998] Add optional exception handler to flink-connector-opensearch
Posted by "lilyevsky (via GitHub)" <gi...@apache.org>.
lilyevsky commented on PR #9:
URL: https://github.com/apache/flink-connector-opensearch/pull/9#issuecomment-1432188777
I will close this PR and create another one. I did make the fixes according to your suggestions.
--
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