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 2021/02/07 18:13:49 UTC
[GitHub] [kafka] chia7712 opened a new pull request #10078: MINOR: add missed versions to clients tests
chia7712 opened a new pull request #10078:
URL: https://github.com/apache/kafka/pull/10078
inspired by #10068
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10078: MINOR: make sure all generated data tests cover all versions
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10078:
URL: https://github.com/apache/kafka/pull/10078#discussion_r587450954
##########
File path: clients/src/test/java/org/apache/kafka/common/message/MessageTest.java
##########
@@ -485,7 +485,7 @@ public void testOffsetCommitRequestVersions() throws Exception {
if (version == 1) {
testEquivalentMessageRoundTrip(version, requestData);
} else if (version >= 2 && version <= 4) {
- testAllMessageRoundTripsBetweenVersions(version, (short) 4, requestData, requestData);
+ testAllMessageRoundTripsBetweenVersions(version, (short) 5, requestData, requestData);
Review comment:
`testAllMessageRoundTripsBetweenVersions` exclude the end number so it should pass 5 so as to test version 4
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a change in pull request #10078: MINOR: make sure all generated data tests cover all versions
Posted by GitBox <gi...@apache.org>.
dajac commented on a change in pull request #10078:
URL: https://github.com/apache/kafka/pull/10078#discussion_r587357462
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/ProduceRequestTest.java
##########
@@ -291,19 +292,11 @@ public void testMixedIdempotentData() {
assertTrue(RequestTestUtils.hasIdempotentRecords(request));
}
- private void assertThrowsInvalidRecordExceptionForAllVersions(ProduceRequest.Builder builder) {
- for (short version = builder.oldestAllowedVersion(); version < builder.latestAllowedVersion(); version++) {
- assertThrowsInvalidRecordException(builder, version);
- }
- }
-
- private void assertThrowsInvalidRecordException(ProduceRequest.Builder builder, short version) {
- try {
- builder.build(version).serialize();
- fail("Builder did not raise " + InvalidRecordException.class.getName() + " as expected");
- } catch (RuntimeException e) {
- assertTrue(InvalidRecordException.class.isAssignableFrom(e.getClass()),
- "Unexpected exception type " + e.getClass().getName());
+ private static <T extends Throwable> void assertThrowsInvalidRecordExceptionForAllVersions(ProduceRequest.Builder builder,
Review comment:
Should we rename the method now that the exception is passed as an argument?
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -295,7 +295,7 @@ public void testSerialization() throws Exception {
checkRequest(createDeleteGroupsRequest(), true);
checkErrorResponse(createDeleteGroupsRequest(), unknownServerException, true);
checkResponse(createDeleteGroupsResponse(), 0, true);
- for (int i = 0; i < ApiKeys.LIST_OFFSETS.latestVersion(); i++) {
+ for (int i = 0; i <= ApiKeys.LIST_OFFSETS.latestVersion(); i++) {
Review comment:
Could we use `allVersions` here as well?
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/ProduceRequestTest.java
##########
@@ -291,19 +292,11 @@ public void testMixedIdempotentData() {
assertTrue(RequestTestUtils.hasIdempotentRecords(request));
}
- private void assertThrowsInvalidRecordExceptionForAllVersions(ProduceRequest.Builder builder) {
- for (short version = builder.oldestAllowedVersion(); version < builder.latestAllowedVersion(); version++) {
- assertThrowsInvalidRecordException(builder, version);
- }
- }
-
- private void assertThrowsInvalidRecordException(ProduceRequest.Builder builder, short version) {
- try {
- builder.build(version).serialize();
- fail("Builder did not raise " + InvalidRecordException.class.getName() + " as expected");
- } catch (RuntimeException e) {
- assertTrue(InvalidRecordException.class.isAssignableFrom(e.getClass()),
- "Unexpected exception type " + e.getClass().getName());
+ private static <T extends Throwable> void assertThrowsInvalidRecordExceptionForAllVersions(ProduceRequest.Builder builder,
+ Class<T> expectedType) {
+ for (short version = builder.oldestAllowedVersion(); version <= builder.latestAllowedVersion(); version++) {
+ short v = version;
Review comment:
Could we get rid of `v` and use `version` below directly?
##########
File path: clients/src/test/java/org/apache/kafka/common/message/MessageTest.java
##########
@@ -485,7 +485,7 @@ public void testOffsetCommitRequestVersions() throws Exception {
if (version == 1) {
testEquivalentMessageRoundTrip(version, requestData);
} else if (version >= 2 && version <= 4) {
- testAllMessageRoundTripsBetweenVersions(version, (short) 4, requestData, requestData);
+ testAllMessageRoundTripsBetweenVersions(version, (short) 5, requestData, requestData);
Review comment:
Why are we changing this?
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/OffsetsForLeaderEpochRequestTest.java
##########
@@ -34,15 +34,15 @@ public void testForConsumerRequiresVersion3() {
assertThrows(UnsupportedVersionException.class, () -> builder.build(v));
}
- for (short version = 3; version < ApiKeys.OFFSET_FOR_LEADER_EPOCH.latestVersion(); version++) {
+ for (short version = 3; version <= ApiKeys.OFFSET_FOR_LEADER_EPOCH.latestVersion(); version++) {
OffsetsForLeaderEpochRequest request = builder.build((short) 3);
Review comment:
Should we replace `3` by `version` here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 merged pull request #10078: MINOR: make sure all generated data tests cover all versions
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #10078:
URL: https://github.com/apache/kafka/pull/10078
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10078: MINOR: make sure all generated data tests cover all versions
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10078:
URL: https://github.com/apache/kafka/pull/10078#discussion_r587445495
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/ProduceRequestTest.java
##########
@@ -291,19 +292,11 @@ public void testMixedIdempotentData() {
assertTrue(RequestTestUtils.hasIdempotentRecords(request));
}
- private void assertThrowsInvalidRecordExceptionForAllVersions(ProduceRequest.Builder builder) {
- for (short version = builder.oldestAllowedVersion(); version < builder.latestAllowedVersion(); version++) {
- assertThrowsInvalidRecordException(builder, version);
- }
- }
-
- private void assertThrowsInvalidRecordException(ProduceRequest.Builder builder, short version) {
- try {
- builder.build(version).serialize();
- fail("Builder did not raise " + InvalidRecordException.class.getName() + " as expected");
- } catch (RuntimeException e) {
- assertTrue(InvalidRecordException.class.isAssignableFrom(e.getClass()),
- "Unexpected exception type " + e.getClass().getName());
+ private static <T extends Throwable> void assertThrowsInvalidRecordExceptionForAllVersions(ProduceRequest.Builder builder,
+ Class<T> expectedType) {
+ for (short version = builder.oldestAllowedVersion(); version <= builder.latestAllowedVersion(); version++) {
+ short v = version;
Review comment:
It requires final variable. I rewrite code by lambda foreach to eliminate duplicate intermediate variable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10078: MINOR: Some tests in clients module does not cover all available versions
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10078:
URL: https://github.com/apache/kafka/pull/10078#discussion_r586408680
##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -201,7 +201,7 @@ public short oldestVersion() {
public List<Short> allVersions() {
List<Short> versions = new ArrayList<>(latestVersion() - oldestVersion() + 1);
- for (short version = oldestVersion(); version < latestVersion(); version++) {
+ for (short version = oldestVersion(); version <= latestVersion(); version++) {
Review comment:
This is not a critical bug as the method `allVersions` is used by testing only.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10078: MINOR: make sure all generated data tests cover all versions
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10078:
URL: https://github.com/apache/kafka/pull/10078#discussion_r587443165
##########
File path: clients/src/test/java/org/apache/kafka/common/requests/OffsetsForLeaderEpochRequestTest.java
##########
@@ -34,15 +34,15 @@ public void testForConsumerRequiresVersion3() {
assertThrows(UnsupportedVersionException.class, () -> builder.build(v));
}
- for (short version = 3; version < ApiKeys.OFFSET_FOR_LEADER_EPOCH.latestVersion(); version++) {
+ for (short version = 3; version <= ApiKeys.OFFSET_FOR_LEADER_EPOCH.latestVersion(); version++) {
OffsetsForLeaderEpochRequest request = builder.build((short) 3);
Review comment:
good catch. fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org