You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "jolshan (via GitHub)" <gi...@apache.org> on 2023/10/17 22:46:16 UTC
[PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
jolshan opened a new pull request, #14568:
URL: https://github.com/apache/kafka/pull/14568
I've added a new class with an incrementing atomic long to represent the verification guard. Upon creation of verification guard, we will increment this value and assign it to the guard.
The expected behavior is the same as the object guard, but with better debuggability with the string value and type safety (I found a type safety issue in the current code when implementing this)
### 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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1363840779
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+ private static final AtomicLong INCREMENTING_ID = new AtomicLong(0L);
+ private final long verificationGuardValue;
+
+ public VerificationGuard() {
+ verificationGuardValue = INCREMENTING_ID.incrementAndGet();
+ }
+
+ @Override
+ public String toString() {
+ return "VerificationGuard: " + verificationGuardValue;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if ((null == obj) || (obj.getClass() != this.getClass()))
+ return false;
+ VerificationGuard guard = (VerificationGuard) obj;
+ return verificationGuardValue == guard.verificationGuardValue();
+ }
+
+ @Override
+ public int hashCode() {
+ long value = verificationGuardValue;
+ return (int) (value ^ (value >>> 32));
Review Comment:
You can use Long.hashCode().
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1363844237
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -1080,9 +1080,9 @@ class UnifiedLog(@volatile var logStartOffset: Long,
(updatedProducers, completedTxns.toList, None)
}
- private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: Object): Boolean = {
+ private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: VerificationGuard): Boolean = {
producerStateManager.producerStateManagerConfig().transactionVerificationEnabled() && !batch.isControlBatch &&
- (requestVerificationGuard != verificationGuard(batch.producerId) || requestVerificationGuard == null)
+ (requestVerificationGuard == null || !requestVerificationGuard.equals(verificationGuard(batch.producerId)))
Review Comment:
Both implementations are correct. Personally I'd go with Justine's approach since the guard implements equals/hash code/toString. Static code checkers and humans would typically be surprised at reference quality being used in such cases.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1364117639
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -1080,9 +1080,9 @@ class UnifiedLog(@volatile var logStartOffset: Long,
(updatedProducers, completedTxns.toList, None)
}
- private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: Object): Boolean = {
+ private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: VerificationGuard): Boolean = {
producerStateManager.producerStateManagerConfig().transactionVerificationEnabled() && !batch.isControlBatch &&
- (requestVerificationGuard != verificationGuard(batch.producerId) || requestVerificationGuard == null)
+ (requestVerificationGuard == null || !requestVerificationGuard.equals(verificationGuard(batch.producerId)))
Review Comment:
The sentinel could be value==0. I think that would be a lot cleaner.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1364117639
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -1080,9 +1080,9 @@ class UnifiedLog(@volatile var logStartOffset: Long,
(updatedProducers, completedTxns.toList, None)
}
- private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: Object): Boolean = {
+ private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: VerificationGuard): Boolean = {
producerStateManager.producerStateManagerConfig().transactionVerificationEnabled() && !batch.isControlBatch &&
- (requestVerificationGuard != verificationGuard(batch.producerId) || requestVerificationGuard == null)
+ (requestVerificationGuard == null || !requestVerificationGuard.equals(verificationGuard(batch.producerId)))
Review Comment:
The sentinel could be value==0. I think using a sentinel would be cleaner.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1363844237
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -1080,9 +1080,9 @@ class UnifiedLog(@volatile var logStartOffset: Long,
(updatedProducers, completedTxns.toList, None)
}
- private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: Object): Boolean = {
+ private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: VerificationGuard): Boolean = {
producerStateManager.producerStateManagerConfig().transactionVerificationEnabled() && !batch.isControlBatch &&
- (requestVerificationGuard != verificationGuard(batch.producerId) || requestVerificationGuard == null)
+ (requestVerificationGuard == null || !requestVerificationGuard.equals(verificationGuard(batch.producerId)))
Review Comment:
Both implementations are correct. Personally I'd go with Justine's approach since the guard implements equals/hash code/toString. Static code checkers and humans would typically be surprised at reference quality being used in such cases (and I don't see a reason to be unusual here).
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1364172007
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -1080,9 +1080,9 @@ class UnifiedLog(@volatile var logStartOffset: Long,
(updatedProducers, completedTxns.toList, None)
}
- private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: Object): Boolean = {
+ private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: VerificationGuard): Boolean = {
producerStateManager.producerStateManagerConfig().transactionVerificationEnabled() && !batch.isControlBatch &&
- (requestVerificationGuard != verificationGuard(batch.producerId) || requestVerificationGuard == null)
+ (requestVerificationGuard == null || !requestVerificationGuard.equals(verificationGuard(batch.producerId)))
Review Comment:
I've updated the code to include the sentinel. We still need to check if the requestVerificationGuard is the sentinel for the case where we enable verification when the request is at the log layer (ie, past the point where we would have made the api call) Without that check, we will get both verification guards as the sentinel and they will be equal.
Alternatively, I can have some methods return null if the verification guard is missing. But that may get confusing.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1364107748
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -1080,9 +1080,9 @@ class UnifiedLog(@volatile var logStartOffset: Long,
(updatedProducers, completedTxns.toList, None)
}
- private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: Object): Boolean = {
+ private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: VerificationGuard): Boolean = {
producerStateManager.producerStateManagerConfig().transactionVerificationEnabled() && !batch.isControlBatch &&
- (requestVerificationGuard != verificationGuard(batch.producerId) || requestVerificationGuard == null)
+ (requestVerificationGuard == null || !requestVerificationGuard.equals(verificationGuard(batch.producerId)))
Review Comment:
We could do that and have the default in other methods be the sentinel. If we create it at startup it will be unique.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1364857293
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its value is always 0.
+ public static final VerificationGuard SENTINEL = new VerificationGuard(0);
+ private static final AtomicLong INCREMENTING_ID = new AtomicLong(0L);
+ private final long value;
+
+ public VerificationGuard() {
+ value = INCREMENTING_ID.incrementAndGet();
+ }
+
+ private VerificationGuard(long value) {
+ this.value = value;
+ }
+
+ @Override
+ public String toString() {
+ return "VerificationGuard: " + value;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if ((null == obj) || (obj.getClass() != this.getClass()))
+ return false;
+ VerificationGuard guard = (VerificationGuard) obj;
+ return value == guard.value();
+ }
+
+ @Override
+ public int hashCode() {
+ return Long.hashCode(value);
+ }
+
+ private long value() {
+ return value;
+ }
+
+ public boolean verifiedBy(VerificationGuard verifyingGuard) {
Review Comment:
Nit: would `verify` be a better method name? It's in the active voice which is generally better for methods.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1365978462
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its value is always 0.
+ public static final VerificationGuard SENTINEL = new VerificationGuard(0);
+ private static final AtomicLong INCREMENTING_ID = new AtomicLong(0L);
+ private final long value;
+
+ public VerificationGuard() {
+ value = INCREMENTING_ID.incrementAndGet();
+ }
+
+ private VerificationGuard(long value) {
+ this.value = value;
+ }
+
+ @Override
+ public String toString() {
+ return "VerificationGuard: " + value;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if ((null == obj) || (obj.getClass() != this.getClass()))
+ return false;
+ VerificationGuard guard = (VerificationGuard) obj;
+ return value == guard.value();
+ }
+
+ @Override
+ public int hashCode() {
+ return Long.hashCode(value);
+ }
+
+ private long value() {
+ return value;
+ }
+
+ public boolean verifiedBy(VerificationGuard verifyingGuard) {
Review Comment:
Map and filter return types that are clearer to their names ie map is usually another structure of the same or a new type and filter is usually the same type.
I guess when you verify something the output is either "yes this was verified or no it is not". So I can see how you could get a boolean there. I could also see verification as an action that is silent and throws an error if it failed.
Maybe I'm just being too grammatically minded here. I would say "x verifies y" and not "x verify y". For map and filter I see it as "map x to z using y" and "filter x to z using y" where x is the instance, y is the argument, and z is the output. For verify it would read as "Verify x using y." It's not a huge deal though so I can just change to verify.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1364343631
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its verificationGuardValue is always 0.
+ public static final VerificationGuard SENTINEL_VERIFICATION_GUARD = new VerificationGuard(0);
Review Comment:
Nit: this applies to many aspects, but we usually don't repeat things like this. For example, we say SENTINEL, not SENTINEL_VERIFICATION_GUARD since the way it's used is VerificationGuard.SENTINEL. Similar for the `verificationGuardValue ` field name and so on.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1365915582
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its value is always 0.
+ public static final VerificationGuard SENTINEL = new VerificationGuard(0);
+ private static final AtomicLong INCREMENTING_ID = new AtomicLong(0L);
+ private final long value;
+
+ public VerificationGuard() {
+ value = INCREMENTING_ID.incrementAndGet();
+ }
+
+ private VerificationGuard(long value) {
+ this.value = value;
+ }
+
+ @Override
+ public String toString() {
+ return "VerificationGuard: " + value;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if ((null == obj) || (obj.getClass() != this.getClass()))
+ return false;
+ VerificationGuard guard = (VerificationGuard) obj;
+ return value == guard.value();
+ }
+
+ @Override
+ public int hashCode() {
+ return Long.hashCode(value);
+ }
+
+ private long value() {
+ return value;
+ }
+
+ public boolean verifiedBy(VerificationGuard verifyingGuard) {
Review Comment:
I guess I also though that it was weird to be an instance method if it is verify. (Verify could be a static method that took two arguments.)
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1365908991
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its value is always 0.
+ public static final VerificationGuard SENTINEL = new VerificationGuard(0);
+ private static final AtomicLong INCREMENTING_ID = new AtomicLong(0L);
+ private final long value;
+
+ public VerificationGuard() {
+ value = INCREMENTING_ID.incrementAndGet();
+ }
+
+ private VerificationGuard(long value) {
+ this.value = value;
+ }
+
+ @Override
+ public String toString() {
+ return "VerificationGuard: " + value;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if ((null == obj) || (obj.getClass() != this.getClass()))
+ return false;
+ VerificationGuard guard = (VerificationGuard) obj;
+ return value == guard.value();
+ }
+
+ @Override
+ public int hashCode() {
+ return Long.hashCode(value);
+ }
+
+ private long value() {
+ return value;
+ }
+
+ public boolean verifiedBy(VerificationGuard verifyingGuard) {
Review Comment:
Interesting, what would you expect `verify` to return? Typically, a given method would return a value that gives you the result for the operation you performed. In this case, it can be a boolean if it's a `true`/`false` case or something more sophisticated if there are more states.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "artemlivshits (via GitHub)" <gi...@apache.org>.
artemlivshits commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1362920242
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -1080,9 +1080,9 @@ class UnifiedLog(@volatile var logStartOffset: Long,
(updatedProducers, completedTxns.toList, None)
}
- private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: Object): Boolean = {
+ private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: VerificationGuard): Boolean = {
producerStateManager.producerStateManagerConfig().transactionVerificationEnabled() && !batch.isControlBatch &&
- (requestVerificationGuard != verificationGuard(batch.producerId) || requestVerificationGuard == null)
+ (requestVerificationGuard == null || !requestVerificationGuard.equals(verificationGuard(batch.producerId)))
Review Comment:
What's the reason to change this logic? Reference comparison works well, handles nulls properly, and we don't have to rely on the autoincremented int other than for diagnostics.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1365594250
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its value is always 0.
+ public static final VerificationGuard SENTINEL = new VerificationGuard(0);
+ private static final AtomicLong INCREMENTING_ID = new AtomicLong(0L);
+ private final long value;
+
+ public VerificationGuard() {
+ value = INCREMENTING_ID.incrementAndGet();
+ }
+
+ private VerificationGuard(long value) {
+ this.value = value;
+ }
+
+ @Override
+ public String toString() {
+ return "VerificationGuard: " + value;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if ((null == obj) || (obj.getClass() != this.getClass()))
+ return false;
+ VerificationGuard guard = (VerificationGuard) obj;
+ return value == guard.value();
+ }
+
+ @Override
+ public int hashCode() {
+ return Long.hashCode(value);
+ }
+
+ private long value() {
+ return value;
+ }
+
+ public boolean verifiedBy(VerificationGuard verifyingGuard) {
Review Comment:
I didn't know if verify sounded like it would return a boolean. But I suppose it can.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1365595431
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its value is always 0.
+ public static final VerificationGuard SENTINEL = new VerificationGuard(0);
+ private static final AtomicLong INCREMENTING_ID = new AtomicLong(0L);
+ private final long value;
+
+ public VerificationGuard() {
+ value = INCREMENTING_ID.incrementAndGet();
+ }
+
+ private VerificationGuard(long value) {
+ this.value = value;
+ }
+
+ @Override
+ public String toString() {
+ return "VerificationGuard: " + value;
Review Comment:
I thought this was a little off too, but couldn't think of a good alternative. Thanks for the suggestions! I will use on of 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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1364343631
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its verificationGuardValue is always 0.
+ public static final VerificationGuard SENTINEL_VERIFICATION_GUARD = new VerificationGuard(0);
Review Comment:
Nit: this applies to many aspects, but we usually don't repeat things like this. For example, we say SENTINEL, not SENTINEL_VERIFICATION_GUARD since the way it's used is VerificationGuard.SENTINEL. Similar for the `value` field name and so on.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1365914738
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its value is always 0.
+ public static final VerificationGuard SENTINEL = new VerificationGuard(0);
+ private static final AtomicLong INCREMENTING_ID = new AtomicLong(0L);
+ private final long value;
+
+ public VerificationGuard() {
+ value = INCREMENTING_ID.incrementAndGet();
+ }
+
+ private VerificationGuard(long value) {
+ this.value = value;
+ }
+
+ @Override
+ public String toString() {
+ return "VerificationGuard: " + value;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if ((null == obj) || (obj.getClass() != this.getClass()))
+ return false;
+ VerificationGuard guard = (VerificationGuard) obj;
+ return value == guard.value();
+ }
+
+ @Override
+ public int hashCode() {
+ return Long.hashCode(value);
+ }
+
+ private long value() {
+ return value;
+ }
+
+ public boolean verifiedBy(VerificationGuard verifyingGuard) {
Review Comment:
I guess it sounded more like a void/unit type method that does an action. But yeah, then I guess the result is true/false if the action had a conclusion. I can change to verify if we think that is best.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "artemlivshits (via GitHub)" <gi...@apache.org>.
artemlivshits commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1362959888
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -1080,9 +1080,9 @@ class UnifiedLog(@volatile var logStartOffset: Long,
(updatedProducers, completedTxns.toList, None)
}
- private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: Object): Boolean = {
+ private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: VerificationGuard): Boolean = {
producerStateManager.producerStateManagerConfig().transactionVerificationEnabled() && !batch.isControlBatch &&
- (requestVerificationGuard != verificationGuard(batch.producerId) || requestVerificationGuard == null)
+ (requestVerificationGuard == null || !requestVerificationGuard.equals(verificationGuard(batch.producerId)))
Review Comment:
It's not possible to have a collision on comparison of references of 2 separately allocated live objects (unless there is a bug in JVM), there could be collision on the hash codes, but not references. And it's not just because reference is 64 bits, the references of 2 live objects are never the same, this is guaranteed by JVM. It's theoretically possible to get a collision on integers (they are just auto-incremented, after all, nobody tracks that lifetime of an object with one id doesn't overlap with lifetime with object of collided id), though it would require a very long time for a 64 bit integer (shouldn't happen in practice in lifetime of a broker).
We definitely have examples of this in Kafka codebase https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/producer/internals/BuiltInPartitioner.java#L164
This one is written by me :-), but I'm pretty sure I've seen this pattern somewhere else in Kafka.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1365978462
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its value is always 0.
+ public static final VerificationGuard SENTINEL = new VerificationGuard(0);
+ private static final AtomicLong INCREMENTING_ID = new AtomicLong(0L);
+ private final long value;
+
+ public VerificationGuard() {
+ value = INCREMENTING_ID.incrementAndGet();
+ }
+
+ private VerificationGuard(long value) {
+ this.value = value;
+ }
+
+ @Override
+ public String toString() {
+ return "VerificationGuard: " + value;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if ((null == obj) || (obj.getClass() != this.getClass()))
+ return false;
+ VerificationGuard guard = (VerificationGuard) obj;
+ return value == guard.value();
+ }
+
+ @Override
+ public int hashCode() {
+ return Long.hashCode(value);
+ }
+
+ private long value() {
+ return value;
+ }
+
+ public boolean verifiedBy(VerificationGuard verifyingGuard) {
Review Comment:
Map and filter return types that are clearer to me, but maybe I'm just more familiar.
I guess when you verify something the output is either "yes this was verified or no it is not". So I can see how you could get a boolean there. I could also see verification as an action that is silent and throws an error if it failed.
Maybe I'm just being too grammatically minded here. I would say "x verifies y" and not "x verify y". For map and filter I see it as "map x to z using y" and "filter x to z using y" where x is the instance, y is the argument, and z is the output. For verify it would read as "Verify x using y." It's not a huge deal though so I can just change to verify.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1363844237
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -1080,9 +1080,9 @@ class UnifiedLog(@volatile var logStartOffset: Long,
(updatedProducers, completedTxns.toList, None)
}
- private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: Object): Boolean = {
+ private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: VerificationGuard): Boolean = {
producerStateManager.producerStateManagerConfig().transactionVerificationEnabled() && !batch.isControlBatch &&
- (requestVerificationGuard != verificationGuard(batch.producerId) || requestVerificationGuard == null)
+ (requestVerificationGuard == null || !requestVerificationGuard.equals(verificationGuard(batch.producerId)))
Review Comment:
Both implementations are correct. Personally I'd go with Justine's approach since the guard implements equals/hashCode/toString. Static code checkers and humans would typically be surprised at reference quality being used in such cases (and I don't see a reason to be unusual here).
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1362938647
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -1080,9 +1080,9 @@ class UnifiedLog(@volatile var logStartOffset: Long,
(updatedProducers, completedTxns.toList, None)
}
- private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: Object): Boolean = {
+ private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: VerificationGuard): Boolean = {
producerStateManager.producerStateManagerConfig().transactionVerificationEnabled() && !batch.isControlBatch &&
- (requestVerificationGuard != verificationGuard(batch.producerId) || requestVerificationGuard == null)
+ (requestVerificationGuard == null || !requestVerificationGuard.equals(verificationGuard(batch.producerId)))
Review Comment:
I recall part of the conversation referring to the likelihood of collisions. I see that the reference is also 64 bits.
I can change it back to reference, but since we have the autoincrementing value, I thought it could be useful for readability of the code since object references being used to check equality aren't as common of a pattern in Java.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1365876383
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its value is always 0.
+ public static final VerificationGuard SENTINEL = new VerificationGuard(0);
+ private static final AtomicLong INCREMENTING_ID = new AtomicLong(0L);
+ private final long value;
+
+ public VerificationGuard() {
+ value = INCREMENTING_ID.incrementAndGet();
+ }
+
+ private VerificationGuard(long value) {
+ this.value = value;
+ }
+
+ @Override
+ public String toString() {
+ return "VerificationGuard: " + value;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if ((null == obj) || (obj.getClass() != this.getClass()))
+ return false;
+ VerificationGuard guard = (VerificationGuard) obj;
+ return value == guard.value();
+ }
+
+ @Override
+ public int hashCode() {
+ return Long.hashCode(value);
+ }
+
+ private long value() {
+ return value;
+ }
+
+ public boolean verifiedBy(VerificationGuard verifyingGuard) {
Review Comment:
Another active voice option is "verifies" (kind of like "equals").
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1365968314
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its value is always 0.
+ public static final VerificationGuard SENTINEL = new VerificationGuard(0);
+ private static final AtomicLong INCREMENTING_ID = new AtomicLong(0L);
+ private final long value;
+
+ public VerificationGuard() {
+ value = INCREMENTING_ID.incrementAndGet();
+ }
+
+ private VerificationGuard(long value) {
+ this.value = value;
+ }
+
+ @Override
+ public String toString() {
+ return "VerificationGuard: " + value;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if ((null == obj) || (obj.getClass() != this.getClass()))
+ return false;
+ VerificationGuard guard = (VerificationGuard) obj;
+ return value == guard.value();
+ }
+
+ @Override
+ public int hashCode() {
+ return Long.hashCode(value);
+ }
+
+ private long value() {
+ return value;
+ }
+
+ public boolean verifiedBy(VerificationGuard verifyingGuard) {
Review Comment:
How is `verify` different than `map`, `filter` and other similar instance methods? I am asking these questions because naming conventions are important overall, not due to this one example.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1364207881
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -1080,9 +1080,9 @@ class UnifiedLog(@volatile var logStartOffset: Long,
(updatedProducers, completedTxns.toList, None)
}
- private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: Object): Boolean = {
+ private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: VerificationGuard): Boolean = {
producerStateManager.producerStateManagerConfig().transactionVerificationEnabled() && !batch.isControlBatch &&
- (requestVerificationGuard != verificationGuard(batch.producerId) || requestVerificationGuard == null)
+ (requestVerificationGuard == null || !requestVerificationGuard.equals(verificationGuard(batch.producerId)))
Review Comment:
I think you'd actually want to have a method in `VerificationGuard` to do the comparison, not `equals`. That method would then check for the sentinel and do the right thing. Does that make sense?
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1363846193
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -1080,9 +1080,9 @@ class UnifiedLog(@volatile var logStartOffset: Long,
(updatedProducers, completedTxns.toList, None)
}
- private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: Object): Boolean = {
+ private def batchMissingRequiredVerification(batch: MutableRecordBatch, requestVerificationGuard: VerificationGuard): Boolean = {
producerStateManager.producerStateManagerConfig().transactionVerificationEnabled() && !batch.isControlBatch &&
- (requestVerificationGuard != verificationGuard(batch.producerId) || requestVerificationGuard == null)
+ (requestVerificationGuard == null || !requestVerificationGuard.equals(verificationGuard(batch.producerId)))
Review Comment:
The second question is: can we avoid the null check altogether by using a sentinel?
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "ijuma (via GitHub)" <gi...@apache.org>.
ijuma commented on code in PR #14568:
URL: https://github.com/apache/kafka/pull/14568#discussion_r1364857293
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its value is always 0.
+ public static final VerificationGuard SENTINEL = new VerificationGuard(0);
+ private static final AtomicLong INCREMENTING_ID = new AtomicLong(0L);
+ private final long value;
+
+ public VerificationGuard() {
+ value = INCREMENTING_ID.incrementAndGet();
+ }
+
+ private VerificationGuard(long value) {
+ this.value = value;
+ }
+
+ @Override
+ public String toString() {
+ return "VerificationGuard: " + value;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if ((null == obj) || (obj.getClass() != this.getClass()))
+ return false;
+ VerificationGuard guard = (VerificationGuard) obj;
+ return value == guard.value();
+ }
+
+ @Override
+ public int hashCode() {
+ return Long.hashCode(value);
+ }
+
+ private long value() {
+ return value;
+ }
+
+ public boolean verifiedBy(VerificationGuard verifyingGuard) {
Review Comment:
Nit: would `verify` be a better method name? It's the active voice which is generally better for methods.
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
Review Comment:
Nit: should this be final?
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/VerificationGuard.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.storage.internals.log;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+public class VerificationGuard {
+
+ // The sentinel VerificationGuard will be used as a default when no verification guard is provided.
+ // It can not be used to verify a transaction is ongoing and its value is always 0.
+ public static final VerificationGuard SENTINEL = new VerificationGuard(0);
+ private static final AtomicLong INCREMENTING_ID = new AtomicLong(0L);
+ private final long value;
+
+ public VerificationGuard() {
+ value = INCREMENTING_ID.incrementAndGet();
+ }
+
+ private VerificationGuard(long value) {
+ this.value = value;
+ }
+
+ @Override
+ public String toString() {
+ return "VerificationGuard: " + value;
Review Comment:
Nit: I think `VerificationGuard(2)` or `VerificationGuard(value=2)` (if value is 2) might be a more idiomatic way of implementing this.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on PR #14568:
URL: https://github.com/apache/kafka/pull/14568#issuecomment-1773413391
Test failures look unrelated. Will merge.
--
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
Re: [PR] KAFKA-15626: Replace verification guard object with an specific type [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan merged PR #14568:
URL: https://github.com/apache/kafka/pull/14568
--
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