Skip to content

KAFKA-6870 Concurrency conflicts in SampledStat#4985

Merged
rajinisivaram merged 3 commits intoapache:trunkfrom
chia7712:kafka_6870
May 10, 2018
Merged

KAFKA-6870 Concurrency conflicts in SampledStat#4985
rajinisivaram merged 3 commits intoapache:trunkfrom
chia7712:kafka_6870

Conversation

@chia7712
Copy link
Copy Markdown
Member

@chia7712 chia7712 commented May 9, 2018

see KAFKA-6870 for more details.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@rajinisivaram
Copy link
Copy Markdown
Contributor

@chia7712 Can you add one unit test please?

@chia7712
Copy link
Copy Markdown
Member Author

chia7712 commented May 9, 2018

Can you add one unit test please?

Copy that.

Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chia7712 Thanks for the PR. Looks good, left a few minor comments.

return ((Measurable) metricValueProvider).measure(config, timeMs);
else
return 0;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: unnecessary newline

}));
}
latch.countDown();
service.shutdown();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to shutdown the executor service even if there is a test failure (either in a finally block or the teardown).

assertTrue(service.awaitTermination(10, TimeUnit.SECONDS));
for (Future<Throwable> callable : workers) {
assertTrue(callable.isDone());
assertNull(callable.get());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add error messages for these assertions?

@chia7712
Copy link
Copy Markdown
Member Author

Thanks for the reviews. @rajinisivaram I have addressed you comment in the latest commit. Please take a look.

Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chia7712 Thanks for the update, LGTM. Will merge after PR builds complete.

Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chia7712 Thanks for the PR, merging to trunk and 1.1.

@rajinisivaram rajinisivaram merged commit 4f7c11a into apache:trunk May 10, 2018
rajinisivaram pushed a commit that referenced this pull request May 10, 2018
Make `KafkaMetric.measurableValue` thread-safe

Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>
ijuma added a commit to ijuma/kafka that referenced this pull request May 11, 2018
…-record-version

* apache-github/trunk:
  KAFKA-6894: Improve err msg when connecting processor with global store (apache#5000)
  KAFKA-6893; Create processors before starting acceptor in SocketServer (apache#4999)
  MINOR: Fix typo in ConsumerRebalanceListener JavaDoc (apache#4996)
  MINOR: Remove deprecated valueTransformer.punctuate (apache#4993)
  MINOR: Update dynamic broker configuration doc for truststore update (apache#4954)
  KAFKA-6870 Concurrency conflicts in SampledStat (apache#4985)
  KAFKA-6361: Fix log divergence between leader and follower after fast leader fail over (apache#4882)
  KAFKA-6813: Remove deprecated APIs in KIP-182, Part II (apache#4976)
  KAFKA-6878 Switch the order of underlying.init and initInternal (apache#4988)
  KAFKA-6299; Fix AdminClient error handling when metadata changes (apache#4295)
  KAFKA-6878: NPE when querying global state store not in READY state (apache#4978)
  KAFKA 6673: Implemented missing override equals method (apache#4745)
  KAFKA-6834: Handle compaction with batches bigger than max.message.bytes (apache#4953)
ying-zheng pushed a commit to ying-zheng/kafka that referenced this pull request Jul 6, 2018
Make `KafkaMetric.measurableValue` thread-safe

Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>
@chia7712 chia7712 deleted the kafka_6870 branch March 25, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants