You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "fvaleri (via GitHub)" <gi...@apache.org> on 2023/05/31 05:58:21 UTC
[GitHub] [kafka] fvaleri opened a new pull request, #13784: [MINOR] Fix for MetadataQuorumCommandErrorTest.testRelativeTimeMs
fvaleri opened a new pull request, #13784:
URL: https://github.com/apache/kafka/pull/13784
This change should fix the following error on CI/CD.
```sh
org.apache.kafka.common.KafkaException: Error while computing relative time, possible drift in system clock.
Current timestamp is 1685472793724, test timestamp is 1685472793724
```
--
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] dajac commented on a diff in pull request #13784: MINOR: Fix for MetadataQuorumCommandErrorTest.testRelativeTimeMs
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13784:
URL: https://github.com/apache/kafka/pull/13784#discussion_r1211158842
##########
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandErrorTest.java:
##########
@@ -53,12 +53,12 @@ public void testDescribeOptions() {
@Test
public void testRelativeTimeMs() {
- long nowMs = Instant.now().toEpochMilli();
- assertTrue(MetadataQuorumCommand.relativeTimeMs(nowMs, "test") >= 0);
- long invalidEpochMs = Instant.EPOCH.minus(1, ChronoUnit.DAYS).toEpochMilli();
+ long validEpochMs = Instant.now().minusSeconds(5).toEpochMilli();
+ assertTrue(MetadataQuorumCommand.relativeTimeMs(validEpochMs, "test") >= 0);
+ long invalidEpochMs = Instant.EPOCH.minusSeconds(86400).toEpochMilli();
assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(invalidEpochMs, "test"));
- long futureTimestampMs = Instant.now().plus(1, ChronoUnit.DAYS).toEpochMilli();
- assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(futureTimestampMs, "test"));
+ long futureEpochMs = Instant.now().plus(1, ChronoUnit.DAYS).toEpochMilli();
+ assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(futureEpochMs, "test"));
Review Comment:
I wonder if this is the right way to fix it. My understanding is that `relativeTimeMs` fails if the provided timestamp is not strictly greater than the current time. Should we loosen this validation to accept a timestamp greater than or equals to the current 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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] fvaleri commented on a diff in pull request #13784: MINOR: Fix for MetadataQuorumCommandErrorTest.testRelativeTimeMs
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on code in PR #13784:
URL: https://github.com/apache/kafka/pull/13784#discussion_r1211492270
##########
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandErrorTest.java:
##########
@@ -53,12 +53,14 @@ public void testDescribeOptions() {
@Test
public void testRelativeTimeMs() {
+ long validEpochMs = Instant.now().minusSeconds(5).toEpochMilli();
+ assertTrue(MetadataQuorumCommand.relativeTimeMs(validEpochMs, "test") >= 0);
long nowMs = Instant.now().toEpochMilli();
assertTrue(MetadataQuorumCommand.relativeTimeMs(nowMs, "test") >= 0);
- long invalidEpochMs = Instant.EPOCH.minus(1, ChronoUnit.DAYS).toEpochMilli();
+ long invalidEpochMs = Instant.EPOCH.minusSeconds(86400).toEpochMilli();
Review Comment:
Right, I'll revert once the CI run ends. 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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] fvaleri commented on a diff in pull request #13784: MINOR: Fix for MetadataQuorumCommandErrorTest.testRelativeTimeMs
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on code in PR #13784:
URL: https://github.com/apache/kafka/pull/13784#discussion_r1211224639
##########
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandErrorTest.java:
##########
@@ -53,12 +53,12 @@ public void testDescribeOptions() {
@Test
public void testRelativeTimeMs() {
- long nowMs = Instant.now().toEpochMilli();
- assertTrue(MetadataQuorumCommand.relativeTimeMs(nowMs, "test") >= 0);
- long invalidEpochMs = Instant.EPOCH.minus(1, ChronoUnit.DAYS).toEpochMilli();
+ long validEpochMs = Instant.now().minusSeconds(5).toEpochMilli();
+ assertTrue(MetadataQuorumCommand.relativeTimeMs(validEpochMs, "test") >= 0);
+ long invalidEpochMs = Instant.EPOCH.minusSeconds(86400).toEpochMilli();
assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(invalidEpochMs, "test"));
- long futureTimestampMs = Instant.now().plus(1, ChronoUnit.DAYS).toEpochMilli();
- assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(futureTimestampMs, "test"));
+ long futureEpochMs = Instant.now().plus(1, ChronoUnit.DAYS).toEpochMilli();
+ assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(futureEpochMs, "test"));
Review Comment:
Yeah, I was thinking the same. The check now is `epoch < timestamp < now`, but it should be `epoch < timestamp <= now`. In the last commit I fixed `relativeTimeMs` and added back the `nowMs` check. Thanks for the review.
##########
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandErrorTest.java:
##########
@@ -53,12 +53,12 @@ public void testDescribeOptions() {
@Test
public void testRelativeTimeMs() {
- long nowMs = Instant.now().toEpochMilli();
- assertTrue(MetadataQuorumCommand.relativeTimeMs(nowMs, "test") >= 0);
- long invalidEpochMs = Instant.EPOCH.minus(1, ChronoUnit.DAYS).toEpochMilli();
+ long validEpochMs = Instant.now().minusSeconds(5).toEpochMilli();
+ assertTrue(MetadataQuorumCommand.relativeTimeMs(validEpochMs, "test") >= 0);
+ long invalidEpochMs = Instant.EPOCH.minusSeconds(86400).toEpochMilli();
assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(invalidEpochMs, "test"));
- long futureTimestampMs = Instant.now().plus(1, ChronoUnit.DAYS).toEpochMilli();
- assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(futureTimestampMs, "test"));
+ long futureEpochMs = Instant.now().plus(1, ChronoUnit.DAYS).toEpochMilli();
+ assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(futureEpochMs, "test"));
Review Comment:
Yeah, I was thinking the same. The check now is `epoch < timestamp < now`, but it should be `epoch < timestamp <= now`. In the last commit I fixed `relativeTimeMs` and added back the `nowMs` check.
Thanks for the review.
--
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] divijvaidya commented on a diff in pull request #13784: MINOR: Fix for MetadataQuorumCommandErrorTest.testRelativeTimeMs
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13784:
URL: https://github.com/apache/kafka/pull/13784#discussion_r1211479979
##########
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandErrorTest.java:
##########
@@ -53,12 +53,14 @@ public void testDescribeOptions() {
@Test
public void testRelativeTimeMs() {
+ long validEpochMs = Instant.now().minusSeconds(5).toEpochMilli();
+ assertTrue(MetadataQuorumCommand.relativeTimeMs(validEpochMs, "test") >= 0);
long nowMs = Instant.now().toEpochMilli();
assertTrue(MetadataQuorumCommand.relativeTimeMs(nowMs, "test") >= 0);
- long invalidEpochMs = Instant.EPOCH.minus(1, ChronoUnit.DAYS).toEpochMilli();
+ long invalidEpochMs = Instant.EPOCH.minusSeconds(86400).toEpochMilli();
Review Comment:
nit
Why did we change this? `minus(1, ChronoUnit.DAYS)` was much more readable than `minusSeconds(86400)`
--
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] fvaleri commented on pull request #13784: MINOR: Fix for MetadataQuorumCommandErrorTest.testRelativeTimeMs
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on PR #13784:
URL: https://github.com/apache/kafka/pull/13784#issuecomment-1570034444
@dajac @divijvaidya the fix should be ready now. Thanks.
https://ci-builds.apache.org/blue/rest/organizations/jenkins/pipelines/Kafka/pipelines/kafka-pr/branches/PR-13784/runs/3/nodes/13/log/?start=0
--
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] dajac merged pull request #13784: MINOR: Fix for MetadataQuorumCommandErrorTest.testRelativeTimeMs
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac merged PR #13784:
URL: https://github.com/apache/kafka/pull/13784
--
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] fvaleri commented on a diff in pull request #13784: MINOR: Fix for MetadataQuorumCommandErrorTest.testRelativeTimeMs
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on code in PR #13784:
URL: https://github.com/apache/kafka/pull/13784#discussion_r1211224639
##########
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandErrorTest.java:
##########
@@ -53,12 +53,12 @@ public void testDescribeOptions() {
@Test
public void testRelativeTimeMs() {
- long nowMs = Instant.now().toEpochMilli();
- assertTrue(MetadataQuorumCommand.relativeTimeMs(nowMs, "test") >= 0);
- long invalidEpochMs = Instant.EPOCH.minus(1, ChronoUnit.DAYS).toEpochMilli();
+ long validEpochMs = Instant.now().minusSeconds(5).toEpochMilli();
+ assertTrue(MetadataQuorumCommand.relativeTimeMs(validEpochMs, "test") >= 0);
+ long invalidEpochMs = Instant.EPOCH.minusSeconds(86400).toEpochMilli();
assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(invalidEpochMs, "test"));
- long futureTimestampMs = Instant.now().plus(1, ChronoUnit.DAYS).toEpochMilli();
- assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(futureTimestampMs, "test"));
+ long futureEpochMs = Instant.now().plus(1, ChronoUnit.DAYS).toEpochMilli();
+ assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(futureEpochMs, "test"));
Review Comment:
Yeah, I was thinking the same. The check now is `epoch < timestamp < now`, but it should be `epoch < timestamp <= now`. In the last commit I fixed `relativeTimeMs` and added back the `nowMs` check. Let's wait for CI/CD outcome.
Thanks for the review.
##########
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandErrorTest.java:
##########
@@ -53,12 +53,12 @@ public void testDescribeOptions() {
@Test
public void testRelativeTimeMs() {
- long nowMs = Instant.now().toEpochMilli();
- assertTrue(MetadataQuorumCommand.relativeTimeMs(nowMs, "test") >= 0);
- long invalidEpochMs = Instant.EPOCH.minus(1, ChronoUnit.DAYS).toEpochMilli();
+ long validEpochMs = Instant.now().minusSeconds(5).toEpochMilli();
+ assertTrue(MetadataQuorumCommand.relativeTimeMs(validEpochMs, "test") >= 0);
+ long invalidEpochMs = Instant.EPOCH.minusSeconds(86400).toEpochMilli();
assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(invalidEpochMs, "test"));
- long futureTimestampMs = Instant.now().plus(1, ChronoUnit.DAYS).toEpochMilli();
- assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(futureTimestampMs, "test"));
+ long futureEpochMs = Instant.now().plus(1, ChronoUnit.DAYS).toEpochMilli();
+ assertThrows(KafkaException.class, () -> MetadataQuorumCommand.relativeTimeMs(futureEpochMs, "test"));
Review Comment:
Yeah, I was thinking the same. The check now is `epoch < timestamp < now`, but it should be `epoch < timestamp <= now`. In the last commit I fixed `relativeTimeMs` and added back the `nowMs` check. Let's wait for CI outcome.
Thanks for the review.
--
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