Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
MartijnVisser merged PR #24249: URL: https://github.com/apache/flink/pull/24249 -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
MartijnVisser commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1930591058 @flinkbot run azure -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
MartijnVisser commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1930084441 > Sorry for the late response, shall I rebase the master and rerun the CI? Yes, that would be great! -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
Jiabao-Sun commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1930080330 Sorry for the late response, shall I rebase the master and rerun the CI? -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
XComp commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1930073322 Your conclusion is correct. The error in the [CI run](https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=57346=logs=bea52777-eaf8-5663-8482-18fbc3630e81=43ba8ce7-ebbf-57cd-9163-444305d74117=8405) is due to a out-dated branch base. A more recent version of `master` that includes https://github.com/apache/flink/commit/79cccd7103a304bfa07104dcafd1f65a032c88ce (which is needed for the resolution of FLINK-34007) would fix the issue. :+1: -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
lincoln-lil commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1930042198 I checked https://issues.apache.org/jira/browse/FLINK-34007 and there's a same failure stack before the final fix merged https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=57270=logs=bbb1e2a2-a43c-55c8-fb48-5cfe7a8a0ca6=a69a379d-ca44-5937-4e62-0ce084a23679=7935 -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
MartijnVisser commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1930037898 > could you rebase the master and rerun the ci? I've at least rebased this PR and triggered a local run on https://dev.azure.com/martijn0323/Flink/_build/results?buildId=4190=results and https://github.com/MartijnVisser/flink/actions/runs/7801927451 -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
lincoln-lil commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1930017946 Seems an unrelated failure here: https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=57346=logs=bea52777-eaf8-5663-8482-18fbc3630e81=43ba8ce7-ebbf-57cd-9163-444305d74117 @Jiabao-Sun Since this pr has 20 commits behind the master (include some fixes related to k8s LeaderElection), could you rebase the master and rerun the ci? cc @XComp Can you help check if this failure case is due to without recent fix? -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
Jiabao-Sun commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1929535430 Thanks @pvary for the review. Please take a look again. -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
pvary commented on PR #24249:
URL: https://github.com/apache/flink/pull/24249#issuecomment-1929260533
Sorry for the late response - I have been otherwise occupied.
Thanks for the test. Could we add a test which checks all of the mappings,
something like this:
```
@Test
void testInitContextWrap() throws Exception {
final AtomicReference initContext = new
AtomicReference<>();
final AtomicReference originalContext = new
AtomicReference<>();
final AtomicBoolean consumed = new AtomicBoolean(false);
final Sink sink =
new Sink() {
@Override
public SinkWriter createWriter(WriterInitContext
context)
throws IOException {
WriterInitContext decoratedContext =
(WriterInitContext)
Proxy.newProxyInstance(
WriterInitContext.class.getClassLoader(),
new Class[]
{WriterInitContext.class},
(proxy, method, args) -> {
if (method.getName()
.equals("metadataConsumer")) {
return Optional.of(
(Consumer)
o ->
consumed.set(true));
}
return
method.invoke(context, args);
});
originalContext.set(decoratedContext);
return Sink.super.createWriter(decoratedContext);
}
@Override
public SinkWriter createWriter(InitContext
context) {
initContext.set(context);
return null;
}
};
final int subtaskId = 1;
final int parallelism = 10;
final TypeSerializer typeSerializer =
StringSerializer.INSTANCE;
final JobID jobID = new JobID();
final MockEnvironment environment =
MockEnvironment.builder()
.setSubtaskIndex(subtaskId)
.setParallelism(parallelism)
.setMaxParallelism(parallelism)
.setJobID(jobID)
.setExecutionConfig(new
ExecutionConfig().enableObjectReuse())
.build();
final OneInputStreamOperatorTestHarness> testHarness =
new OneInputStreamOperatorTestHarness<>(
new SinkWriterOperatorFactory<>(sink),
typeSerializer, environment);
testHarness.open();
assertContextsEqual(initContext.get(), originalContext.get());
testHarness.close();
}
private static void assertContextsEqual(Sink.InitContext initContext,
WriterInitContext original) {
assertThat(initContext.getUserCodeClassLoader().asClassLoader()).isEqualTo(original.getUserCodeClassLoader().asClassLoader());
assertThat(initContext.getMailboxExecutor()).isEqualTo(original.getMailboxExecutor());
assertThat(initContext.getProcessingTimeService()).isEqualTo(original.getProcessingTimeService());
assertThat(initContext.getTaskInfo().getIndexOfThisSubtask()).isEqualTo(original.getTaskInfo().getIndexOfThisSubtask());
assertThat(initContext.getTaskInfo().getNumberOfParallelSubtasks())
.isEqualTo(original.getTaskInfo().getNumberOfParallelSubtasks());
assertThat(initContext.getTaskInfo().getAttemptNumber()).isEqualTo(original.getTaskInfo().getAttemptNumber());
assertThat(initContext.metricGroup()).isEqualTo(original.metricGroup());
assertThat(initContext.getRestoredCheckpointId()).isEqualTo(original.getRestoredCheckpointId());
assertThat(initContext.isObjectReuseEnabled()).isEqualTo(original.isObjectReuseEnabled());
assertThat(initContext.createInputSerializer()).isEqualTo(original.createInputSerializer());
assertThat(initContext.getJobInfo().getJobId()).isEqualTo(original.getJobInfo().getJobId());
assertThat(initContext.metadataConsumer()).isEqualTo(original.metadataConsumer());
}
```
--
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.
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
Jiabao-Sun commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1926664315 Hi @pvary, could you help review it again? Thanks. -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
Jiabao-Sun commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1923360012 Thanks @pvary and @MartijnVisser for the review and I'm trying to add some tests. I discovered this issue while trying to adapt the Kafka connector to the SinkV2 interfaces, which causes the KafkaWriterITCase to fail in 1.19-SNAPSHOT. https://github.com/apache/flink-connector-kafka/blob/abf4563e0342abe25dc28bb6b5457bb971381f61/flink-connector-kafka/src/test/java/org/apache/flink/connector/kafka/sink/KafkaWriterITCase.java#L338 -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
MartijnVisser commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1923352694 Thanks for flagging and fixing this. Like @pvary says, it would be good to see if we could add a test for this. I'll make sure that this gets to the Blockers list for 1.19 -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
pvary commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1923348518 @Jiabao-Sun: Good catch! Thanks! This need to be in 1.19.0! OTOH: > This change is already covered by existing tests. I think this is not true, as there were no unit tests breaking when I forgot to add this method to the wrapper. Which test cover the usage of `metadataConsumer`? Do we want to implement one which breaks if this method is missing? -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
flinkbot commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1922891781 ## CI report: * a57b0e7e04a2f3f3b61a7a5c2ed5f3e8cbae9f47 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
Jiabao-Sun commented on PR #24249: URL: https://github.com/apache/flink/pull/24249#issuecomment-1922891276 Hi @pvary, could you help review it when you have time? -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method [flink]
Jiabao-Sun opened a new pull request, #24249: URL: https://github.com/apache/flink/pull/24249 ## What is the purpose of the change [FLINK-34337][core] Sink.InitContextWrapper should implement metadataConsumer method ## Brief change log Sink.InitContextWrapper should implement metadataConsumer method. If the metadataConsumer method is not implemented, the behavior of the wrapped WriterInitContext's metadataConsumer will be lost. ## Verifying this change Please make sure both new and modified tests in this PR follows the conventions defined in our code quality guide: https://flink.apache.org/contributing/code-style-and-quality-common.html#testing This change is already covered by existing tests. ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (no) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no) - The serializers: (no) - The runtime per-record code paths (performance sensitive): (no) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no) - The S3 file system connector: (no) ## Documentation - Does this pull request introduce a new feature? (no) - If yes, how is the feature documented? (not documented) -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
