You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "satishd (via GitHub)" <gi...@apache.org> on 2023/02/07 11:50:09 UTC
[GitHub] [kafka] satishd opened a new pull request, #13213: KAFKA-14688 Move package org.apache.kafka.server.log.internals to org.apache.kafka.storage.internals.log
satishd opened a new pull request, #13213:
URL: https://github.com/apache/kafka/pull/13213
KAFKA-14688 Move package org.apache.kafka.server.log.internals to org.apache.kafka.storage.internals.log
This is a followup item from https://github.com/apache/kafka/pull/13046 with the [comment]( https://github.com/apache/kafka/pull/13046#discussion_r1063953030)
Changed `AbstractIndex#enrties` from volatile to AtomicInteger as it was getting incremented, which is not a thread safe operation.
### 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] satishd commented on a diff in pull request #13213: KAFKA-14688 Move package org.apache.kafka.server.log.internals to org.apache.kafka.storage.internals.log
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on code in PR #13213:
URL: https://github.com/apache/kafka/pull/13213#discussion_r1098558585
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/AbstractIndex.java:
##########
@@ -64,8 +65,7 @@ private enum SearchResultType {
*/
private volatile int maxEntries;
/** The number of entries in this index */
- private volatile int entries;
-
+ private final AtomicInteger entries;
Review Comment:
Changed from `volatile` to `AtomicInteger` as `entries` is incremented in `incrementEntries` method. This is not a thread safe operation.
--
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] satishd commented on a diff in pull request #13213: KAFKA-14688 Move package org.apache.kafka.server.log.internals to org.apache.kafka.storage.internals.log
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on code in PR #13213:
URL: https://github.com/apache/kafka/pull/13213#discussion_r1098698848
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/AbstractIndex.java:
##########
@@ -64,8 +65,7 @@ private enum SearchResultType {
*/
private volatile int maxEntries;
/** The number of entries in this index */
- private volatile int entries;
-
+ private final AtomicInteger entries;
Review Comment:
I made this change as we were making minor changes when larger refactorings are done in earlier PRs, and calling them out in the PR.
Sure, we can keep this PR only for renames. But I do not think we should have a contract for the caller to invoke in a lock because we have a volatile primtive to be incremented. This can be discussed in another 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishd commented on a diff in pull request #13213: KAFKA-14688 Move package org.apache.kafka.server.log.internals to org.apache.kafka.storage.internals.log
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on code in PR #13213:
URL: https://github.com/apache/kafka/pull/13213#discussion_r1098698848
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/AbstractIndex.java:
##########
@@ -64,8 +65,7 @@ private enum SearchResultType {
*/
private volatile int maxEntries;
/** The number of entries in this index */
- private volatile int entries;
-
+ private final AtomicInteger entries;
Review Comment:
I made this change as we were making minor changes when larger refactorings are done in earlier PRs, and calling out those minor changes in the PR.
Sure, we can keep this PR only for renames. But I do not think we should have a contract for the caller to invoke in a lock because we have a volatile primtive to be incremented. This can be discussed in another 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishd commented on a diff in pull request #13213: KAFKA-14688 Move package org.apache.kafka.server.log.internals to org.apache.kafka.storage.internals.log
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on code in PR #13213:
URL: https://github.com/apache/kafka/pull/13213#discussion_r1098698848
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/AbstractIndex.java:
##########
@@ -64,8 +65,7 @@ private enum SearchResultType {
*/
private volatile int maxEntries;
/** The number of entries in this index */
- private volatile int entries;
-
+ private final AtomicInteger entries;
Review Comment:
I made this change as we were making minor changes when larger refactorings are done in earlier PRs, and calling those minor changes in the PR.
Sure, we can keep this PR only for renames. But I do not think we should have a contract for the caller to invoke in a lock because we have a volatile primtive to be incremented. This can be discussed in another 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishd commented on pull request #13213: KAFKA-14688 Move package org.apache.kafka.server.log.internals to org.apache.kafka.storage.internals.log
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on PR #13213:
URL: https://github.com/apache/kafka/pull/13213#issuecomment-1420880690
Thanks @ijuma for your review. spotBugs task is fixed with the latest commit.
--
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] satishd commented on pull request #13213: KAFKA-14688 Move package org.apache.kafka.server.log.internals to org.apache.kafka.storage.internals.log
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on PR #13213:
URL: https://github.com/apache/kafka/pull/13213#issuecomment-1421954683
Test failure is not related to this change, will merge the changes to trunk.
--
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] ijuma commented on a diff in pull request #13213: KAFKA-14688 Move package org.apache.kafka.server.log.internals to org.apache.kafka.storage.internals.log
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #13213:
URL: https://github.com/apache/kafka/pull/13213#discussion_r1098655061
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/AbstractIndex.java:
##########
@@ -64,8 +65,7 @@ private enum SearchResultType {
*/
private volatile int maxEntries;
/** The number of entries in this index */
- private volatile int entries;
-
+ private final AtomicInteger entries;
Review Comment:
We should not make any changes outside of the rename in this PR. Also, the method `incrementEntries` says:
> // The caller is expected to hold `lock` when calling this method
So, it is actually not incorrect.
--
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] ijuma commented on a diff in pull request #13213: KAFKA-14688 Move package org.apache.kafka.server.log.internals to org.apache.kafka.storage.internals.log
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #13213:
URL: https://github.com/apache/kafka/pull/13213#discussion_r1098706079
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/AbstractIndex.java:
##########
@@ -64,8 +65,7 @@ private enum SearchResultType {
*/
private volatile int maxEntries;
/** The number of entries in this index */
- private volatile int entries;
-
+ private final AtomicInteger entries;
Review Comment:
This PR is different from the others in that it's only a rename and it affects a _lot_ of files. The others were rewriting a smaller set of files from Scala to Java too.
> But I do not think we should have a contract for the caller to invoke in a lock because we have a volatile primtive to be incremented. This can be discussed in another PR.
Debatable, the `incrementEntries` method is `protected` and only used by the two subclasses. The locking protocol of the class is a lot more complicated and I don't see why this particular field is the concern given everything else that is happening in the class.
--
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] satishd merged pull request #13213: KAFKA-14688 Move package org.apache.kafka.server.log.internals to org.apache.kafka.storage.internals.log
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd merged PR #13213:
URL: https://github.com/apache/kafka/pull/13213
--
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