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/03/29 11:07:59 UTC
[GitHub] [kafka] yun-yun opened a new pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
yun-yun opened a new pull request #11963:
URL: https://github.com/apache/kafka/pull/11963
…alized members should be the last step with double-checked locking
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#discussion_r838072037
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/FetchResponse.java
##########
@@ -100,7 +100,8 @@ public Errors error() {
if (responseData == null) {
synchronized (this) {
if (responseData == null) {
- responseData = new LinkedHashMap<>();
+ final LinkedHashMap<TopicPartition, FetchResponseData.PartitionData> responseDataTmp =
Review comment:
nit: Could we add a comment above this line, to mention we are doing this to avoid other threads accessing a half-initialized object, something like that?
--
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] showuon commented on pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#issuecomment-1083096005
I think it should be the infra's issue. I've triggered the jenkins tests again. Let's wait and see.
--
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] yun-yun commented on pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
yun-yun commented on pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#issuecomment-1083012982
@showuon Excuse me, I saw some checks fail. Is there something wrong?
--
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] yun-yun commented on a change in pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
yun-yun commented on a change in pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#discussion_r838231037
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/FetchResponse.java
##########
@@ -100,7 +100,8 @@ public Errors error() {
if (responseData == null) {
synchronized (this) {
if (responseData == null) {
- responseData = new LinkedHashMap<>();
+ final LinkedHashMap<TopicPartition, FetchResponseData.PartitionData> responseDataTmp =
Review comment:
Ok, thank you. Please review it.
--
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 pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#issuecomment-1083100351
I had a quick look at usages of this method and I don't think this causes any issues because we don't really access method concurrently. It is still worth fixing it though. Thanks for the patch! LGTM.
--
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] yun-yun commented on pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
yun-yun commented on pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#issuecomment-1084681688
@showuon Hi, I found the same problem on `FetchRequest#forgottenTopics` and `FetchRequest#fetchData`, could you help confirm and fix them?
--
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] showuon edited a comment on pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#issuecomment-1083096005
I think it should be the infra's issue. I've triggered the jenkins tests again. Let's wait and see.
https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-11963/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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#issuecomment-1083964754
Failed tests are unrelated and also failed in trunk build.
```
Build / PowerPC / org.apache.kafka.clients.consumer.KafkaConsumerTest.testReturnRecordsDuringRebalance()
Build / JDK 11 and Scala 2.13 / kafka.admin.TopicCommandIntegrationTest.testDescribeUnderMinIsrPartitionsMixed()
```
--
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] showuon commented on pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#issuecomment-1083965192
@yun-yun , thanks for the contribution!
--
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] showuon commented on a change in pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#discussion_r838208087
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/FetchResponse.java
##########
@@ -100,7 +100,8 @@ public Errors error() {
if (responseData == null) {
synchronized (this) {
if (responseData == null) {
- responseData = new LinkedHashMap<>();
+ final LinkedHashMap<TopicPartition, FetchResponseData.PartitionData> responseDataTmp =
Review comment:
How about: `Assigning the lazy-initialized responseData in the last step to avoid other threads accessing a half-initialized object.` ?
--
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 pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#issuecomment-1082809733
Do we know if this has any impact?
--
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] yun-yun commented on a change in pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
yun-yun commented on a change in pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#discussion_r838152794
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/FetchResponse.java
##########
@@ -100,7 +100,8 @@ public Errors error() {
if (responseData == null) {
synchronized (this) {
if (responseData == null) {
- responseData = new LinkedHashMap<>();
+ final LinkedHashMap<TopicPartition, FetchResponseData.PartitionData> responseDataTmp =
Review comment:
Thank you for your advice. Since English is not my mother tongue, I would like to write the sentence "We are doing this to avoid other threads accessing a half-initialized object."
--
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] yun-yun commented on pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
yun-yun commented on pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#issuecomment-1083992053
> @yun-yun , thanks for the contribution!
@showuon It's my pleasure
--
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] showuon commented on pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#issuecomment-1082822128
@dajac , the impact I can think of, is that when the issue happened, which should be rare, some threads might get only partial fetch response (some partitions data is ignored), because the first check of `if (responseData == null) {` will be false.
--
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] showuon commented on pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#issuecomment-1085305135
@yun-yun ,Yes, please submit PR to fix them. If you don't have time, you can submit a jira ticket and see if there are other people have time to fix them. 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] yun-yun commented on pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
yun-yun commented on pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#issuecomment-1082829173
@dajac I've seen this in the course of learning, and since locks are used here, maybe you want to avoid exposing uninstantiated objects. personal view.
--
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] showuon merged pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
showuon merged pull request #11963:
URL: https://github.com/apache/kafka/pull/11963
--
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] showuon commented on pull request #11963: KAFKA-13777: Fix FetchResponse#responseData: Assignment of lazy-initialized members should be the last step with double-checked locking
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #11963:
URL: https://github.com/apache/kafka/pull/11963#issuecomment-1082807931
I'll wait for the jenkins build completed before merging it. 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