You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/07/26 20:46:37 UTC
[GitHub] [kafka] C0urante opened a new pull request, #12444: KAFKA-14101: Improve documentation for consuming from embedded Kafka cluster topics in Connect integration testing framework
C0urante opened a new pull request, #12444:
URL: https://github.com/apache/kafka/pull/12444
[Jira](https://issues.apache.org/jira/browse/KAFKA-14101)
Depends on https://github.com/apache/kafka/pull/12429, which should implement a logical fix for KAFKA-14101. This follow-up PR is intended to help harden our integration tests against the mistakes that caused KAFKA-14101 by:
1. Renaming `EmbeddedKafkaCluster::consume` to `EmbeddedKafkaCluster::consumeAtLeast` in order to clarify behavior and help distinguish between it and `EmbeddedKafkaCluster::consumeAll`.
2. Adding a note to the Javadocs for `EmbeddedKafkaCluster::consumeAtLeast` warning about out-of-order consumption and data missing from topic partitions.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] YeonCheolGit commented on a diff in pull request #12444: KAFKA-14101: Improve documentation for consuming from embedded Kafka cluster topics in Connect integration testing framework
Posted by GitBox <gi...@apache.org>.
YeonCheolGit commented on code in PR #12444:
URL: https://github.com/apache/kafka/pull/12444#discussion_r933984891
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExampleConnectIntegrationTest.java:
##########
@@ -157,7 +157,7 @@ public void testSinkConnector() throws Exception {
// consume all records from the source topic or fail, to ensure that they were correctly produced.
Review Comment:
@C0urante
Couldn't agree more!
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a diff in pull request #12444: KAFKA-14101: Improve documentation for consuming from embedded Kafka cluster topics in Connect integration testing framework
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12444:
URL: https://github.com/apache/kafka/pull/12444#discussion_r933894953
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExampleConnectIntegrationTest.java:
##########
@@ -157,7 +157,7 @@ public void testSinkConnector() throws Exception {
// consume all records from the source topic or fail, to ensure that they were correctly produced.
Review Comment:
Ah, good catch! This comment has been copy+pasted across a large number of files:
- `ExampleConnectIntegrationTest`
- `ErrorHandlingIntegrationTest`
- `TransformationIntegrationTest`
- `RebalanceSourceConnectorsIntegrationTest`
In some cases, it's accurate. For example, at this part, we've just manually produced 2000 records into the topic, and we're now re-reading all of those records back to make sure that they were produced successfully.
In other cases, like line 218 in this file, the word "all" is inaccurate since we're consuming from a topic that's being actively written to by a running source connector, which has probably written more than the expected number of records by this point. I think removing "all" from the comment in this case would make it a little ambiguous as to what would cause the method invocation to fail. What do you think about "consume all expected records..." instead of "consume all records"?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] YeonCheolGit commented on a diff in pull request #12444: KAFKA-14101: Improve documentation for consuming from embedded Kafka cluster topics in Connect integration testing framework
Posted by GitBox <gi...@apache.org>.
YeonCheolGit commented on code in PR #12444:
URL: https://github.com/apache/kafka/pull/12444#discussion_r933815183
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExampleConnectIntegrationTest.java:
##########
@@ -157,7 +157,7 @@ public void testSinkConnector() throws Exception {
// consume all records from the source topic or fail, to ensure that they were correctly produced.
Review Comment:
For me `consume all records` comments are different from `consumeAtLeast()` what you are intending for?
Maybe remove `all` ?
```suggestion
// consume records from the source topic or fail, to ensure that they were correctly produced.
```
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/RebalanceSourceConnectorsIntegrationTest.java:
##########
@@ -161,7 +161,7 @@ public void testReconfigConnector() throws Exception {
long recordTransferDurationMs = TimeUnit.SECONDS.toMillis(30);
// consume all records from the source topic or fail, to ensure that they were correctly produced
Review Comment:
Maybe this is the same comment?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] YeonCheolGit commented on a diff in pull request #12444: KAFKA-14101: Improve documentation for consuming from embedded Kafka cluster topics in Connect integration testing framework
Posted by GitBox <gi...@apache.org>.
YeonCheolGit commented on code in PR #12444:
URL: https://github.com/apache/kafka/pull/12444#discussion_r933815183
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExampleConnectIntegrationTest.java:
##########
@@ -157,7 +157,7 @@ public void testSinkConnector() throws Exception {
// consume all records from the source topic or fail, to ensure that they were correctly produced.
Review Comment:
This is super minor thing.
I guess`consume all records` comments are different from `consumeAtLeast()` what you are intending for?
Maybe remove `all` ?
```suggestion
// consume records from the source topic or fail, to ensure that they were correctly produced.
```
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExampleConnectIntegrationTest.java:
##########
@@ -157,7 +157,7 @@ public void testSinkConnector() throws Exception {
// consume all records from the source topic or fail, to ensure that they were correctly produced.
Review Comment:
This is a super minor thing.
I guess`consume all records` comments are different from `consumeAtLeast()` what you are intending for?
Maybe remove `all` ?
```suggestion
// consume records from the source topic or fail, to ensure that they were correctly produced.
```
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on a diff in pull request #12444: KAFKA-14101: Improve documentation for consuming from embedded Kafka cluster topics in Connect integration testing framework
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12444:
URL: https://github.com/apache/kafka/pull/12444#discussion_r934467497
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExampleConnectIntegrationTest.java:
##########
@@ -157,7 +157,7 @@ public void testSinkConnector() throws Exception {
// consume all records from the source topic or fail, to ensure that they were correctly produced.
Review Comment:
👍 done.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] YeonCheolGit commented on a diff in pull request #12444: KAFKA-14101: Improve documentation for consuming from embedded Kafka cluster topics in Connect integration testing framework
Posted by GitBox <gi...@apache.org>.
YeonCheolGit commented on code in PR #12444:
URL: https://github.com/apache/kafka/pull/12444#discussion_r933815183
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExampleConnectIntegrationTest.java:
##########
@@ -157,7 +157,7 @@ public void testSinkConnector() throws Exception {
// consume all records from the source topic or fail, to ensure that they were correctly produced.
Review Comment:
I guess`consume all records` comments are different from `consumeAtLeast()` what you are intending for?
Maybe remove `all` ?
```suggestion
// consume records from the source topic or fail, to ensure that they were correctly produced.
```
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #12444: KAFKA-14101: Improve documentation for consuming from embedded Kafka cluster topics in Connect integration testing framework
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #12444:
URL: https://github.com/apache/kafka/pull/12444#issuecomment-1579167201
Downgrading to a draft until I can revisit and fix the merge conflicts.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org