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 2020/08/24 13:59:54 UTC
[GitHub] [kafka] xakassi opened a new pull request #9211: KAFKA-10426: Deadlock on session key update.
xakassi opened a new pull request #9211:
URL: https://github.com/apache/kafka/pull/9211
DistributedHerder goes to updateConfigsWithIncrementalCooperative() synchronized method and called configBackingStore.snapshot() which take a lock on internal object in KafkaConfigBackingStore class.
Meanwhile KafkaConfigBackingStore in ConsumeCallback inside synchronized block on internal object gets SESSION_KEY record and calls updateListener.onSessionKeyUpdate() which take a lock on DistributedHerder.
So, we have a Deadlock.
To avoid this, updateListener with new session key should be called outside synchronized block as it's done, for example, for updateListener.onTaskConfigUpdate(updatedTasks).
### 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] kkonstantine commented on a change in pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on a change in pull request #9211:
URL: https://github.com/apache/kafka/pull/9211#discussion_r478275661
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
`KafkaConfigBackingStore.this.sessionKey` is already a `volatile` member variable.
So, given that no other common data structures are altered in this block, I'm suspecting that your change means that we might as well get rid off the `synchronized` block altogether in this `else if` branch here.
I'd like to give it a second thought in the morning but lmk what you think.
----------------------------------------------------------------
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] xakassi commented on a change in pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
xakassi commented on a change in pull request #9211:
URL: https://github.com/apache/kafka/pull/9211#discussion_r478325832
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
Hi, @kkonstantine !
Yes, indeed, it looks like we can get rid of the synchronized block here at all. I think it's safe.
I commited this change.
----------------------------------------------------------------
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] kkonstantine commented on pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on pull request #9211:
URL: https://github.com/apache/kafka/pull/9211#issuecomment-701919530
Hi @xakassi
Thanks for addressing the comments. We had some issues with the PR builder at the time, so the tests didn't run.
I'll try to figure out how I could run the tests for this PR and if there's no easy way I'll ask you to submit a new one.
Thanks for your patience.
----------------------------------------------------------------
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] xakassi closed pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
xakassi closed pull request #9211:
URL: https://github.com/apache/kafka/pull/9211
----------------------------------------------------------------
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] xakassi commented on pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
xakassi commented on pull request #9211:
URL: https://github.com/apache/kafka/pull/9211#issuecomment-708231665
I have closed this PR and submitted a new one: https://github.com/apache/kafka/pull/9431
@kkonstantine Please, take a look at it. Thank you!
----------------------------------------------------------------
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] xakassi commented on a change in pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
xakassi commented on a change in pull request #9211:
URL: https://github.com/apache/kafka/pull/9211#discussion_r478325832
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
Hi, @kkonstantine !
Yes, indeed, it looks like we can get rid of the synchronized block here at all. I think it's safe.
I commited this change.
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
Hi, @kkonstantine !
Yes, indeed, it looks like we can get rid of the synchronized block here at all. I think it's safe.
I commited this change.
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
Hi, @kkonstantine !
Yes, indeed, it looks like we can get rid of the synchronized block here at all. I think it's safe.
I commited this change.
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
Hi, @kkonstantine !
Yes, indeed, it looks like we can get rid of the synchronized block here at all. I think it's safe.
I commited this change.
----------------------------------------------------------------
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] xakassi commented on a change in pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
xakassi commented on a change in pull request #9211:
URL: https://github.com/apache/kafka/pull/9211#discussion_r478325832
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
Hi, @kkonstantine !
Yes, indeed, it looks like we can get rid of the synchronized block here at all. I think it's safe.
I commited this change.
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
Hi, @kkonstantine !
Yes, indeed, it looks like we can get rid of the synchronized block here at all. I think it's safe.
I commited this change.
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
Hi, @kkonstantine !
Yes, indeed, it looks like we can get rid of the synchronized block here at all. I think it's safe.
I commited this change.
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
Hi, @kkonstantine !
Yes, indeed, it looks like we can get rid of the synchronized block here at all. I think it's safe.
I commited this change.
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
Hi, @kkonstantine !
Yes, indeed, it looks like we can get rid of the synchronized block here at all. I think it's safe.
I commited this change.
----------------------------------------------------------------
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] xakassi commented on a change in pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
xakassi commented on a change in pull request #9211:
URL: https://github.com/apache/kafka/pull/9211#discussion_r478325832
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
Hi, @kkonstantine !
Yes, indeed, it looks like we can get rid of the synchronized block here at all. I think it's safe.
I commited this change.
----------------------------------------------------------------
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] kkonstantine commented on pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on pull request #9211:
URL: https://github.com/apache/kafka/pull/9211#issuecomment-681830490
ok to test
----------------------------------------------------------------
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] xakassi commented on pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
xakassi commented on pull request #9211:
URL: https://github.com/apache/kafka/pull/9211#issuecomment-707596840
Hi @kkonstantine !
Any news about tests? Should I recreate a PR?
----------------------------------------------------------------
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] xakassi commented on a change in pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
xakassi commented on a change in pull request #9211:
URL: https://github.com/apache/kafka/pull/9211#discussion_r478325832
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
Hi, @kkonstantine !
Yes, indeed, it looks like we can get rid off the synchronized block here at all. I think it's safe.
I will commit this change.
----------------------------------------------------------------
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] xakassi commented on pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
xakassi commented on pull request #9211:
URL: https://github.com/apache/kafka/pull/9211#issuecomment-699805679
Hi, guys! Hi, @kkonstantine !
This is my first PR to Kafka and I would like to clarify if there is anything else I need to do? Or should I just wait and you will take care of this PR?
----------------------------------------------------------------
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] kkonstantine commented on pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
kkonstantine commented on pull request #9211:
URL: https://github.com/apache/kafka/pull/9211#issuecomment-708140200
Yes, let's open a new one @xakassi
I haven't found an easy way to reconnect an old PR with the build job.
Please link to this PR for the comments from the new one. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] xakassi commented on a change in pull request #9211: KAFKA-10426: Deadlock on session key update.
Posted by GitBox <gi...@apache.org>.
xakassi commented on a change in pull request #9211:
URL: https://github.com/apache/kafka/pull/9211#discussion_r478325832
##########
File path: connect/runtime/src/main/java/org/apache/kafka/connect/storage/KafkaConfigBackingStore.java
##########
@@ -733,10 +734,11 @@ public void onCompletion(Throwable error, ConsumerRecord<String, byte[]> record)
new SecretKeySpec(key, (String) keyAlgorithm),
(long) creationTimestamp
);
-
- if (started)
- updateListener.onSessionKeyUpdate(KafkaConfigBackingStore.this.sessionKey);
Review comment:
Hi, @kkonstantine !
Yes, indeed, it looks like we can get rid of the synchronized block here at all. I think it's safe.
I will commit this change.
----------------------------------------------------------------
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