You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "wernerdv (via GitHub)" <gi...@apache.org> on 2024/12/15 15:51:06 UTC
[PR] KAFKA-18209 Cleanup `__transaction_state` config logic [kafka]
wernerdv opened a new pull request, #18201:
URL: https://github.com/apache/kafka/pull/18201
### 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-18209 Cleanup `__transaction_state` config logic [kafka]
Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on PR #18201:
URL: https://github.com/apache/kafka/pull/18201#issuecomment-2718649671
> When I opened the ticket I was hoping we could do a bit more cleanup, but maybe there isn't much to do. The point of the Jira was to investigate and see what could be done.
>
> I left a couple of comments on the changes you propose.
@mimaison If there are any suggestions or comments, I am ready to address them.
If this task is no longer relevant, we can close the PR and Jira.
--
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-18209 Cleanup `__transaction_state` config logic [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #18201:
URL: https://github.com/apache/kafka/pull/18201#discussion_r1940016513
##########
core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala:
##########
@@ -37,14 +38,17 @@ import org.apache.kafka.common.{KafkaException, TopicPartition}
import org.apache.kafka.coordinator.transaction.{TransactionLogConfig, TransactionStateManagerConfig}
import org.apache.kafka.server.common.{RequestLocal, TransactionVersion}
import org.apache.kafka.server.config.ServerConfigs
-import org.apache.kafka.server.record.BrokerCompressionType
import org.apache.kafka.server.storage.log.FetchIsolation
import org.apache.kafka.server.util.Scheduler
import org.apache.kafka.storage.internals.log.AppendOrigin
import scala.jdk.CollectionConverters._
import scala.collection.mutable
+object TransactionStateManager {
+ val EnforcedCompression: Compression = Compression.NONE
+ private val EnforcedRequiredAcks: Short = -1.toShort
Review Comment:
Should we also use this field in `KafkaApis.handleWriteTxnMarkersRequest()`?
##########
core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala:
##########
@@ -400,12 +404,20 @@ class TransactionStateManager(brokerId: Int,
def validateTransactionTimeoutMs(txnTimeoutMs: Int): Boolean =
txnTimeoutMs <= config.transactionMaxTimeoutMs && txnTimeoutMs > 0
+ /**
+ * Enforce always using:
+ * <br>1. cleanup policy = compact
+ * <br>2. compression = none
+ * <br>3. unclean leader election = disabled
+ * <br>4. required acks = -1 when writing
Review Comment:
Not sure why mention `acks` here since it's not related to the topic configurations.
Also _if we really need a list_, we can use an HTML list instead of line returns
##########
core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala:
##########
@@ -45,6 +46,15 @@ import org.apache.kafka.storage.internals.log.AppendOrigin
import scala.jdk.CollectionConverters._
import scala.collection.mutable
+object TransactionStateManager {
+ // enforce always using
+ // 1. cleanup policy = compact
+ // 2. compression = none
+ // 3. unclean leader election = disabled
+ // 4. required acks = -1 when writing
+ val EnforcedCompression: Compression = Compression.NONE
Review Comment:
I'm not sure this is the best place to store these values. Have you considered `TransactionCoordinator`? I'm not saying it should go there, just asking.
--
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-18209 Cleanup `__transaction_state` config logic [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #18201:
URL: https://github.com/apache/kafka/pull/18201#issuecomment-2725101623
Thanks for the updates. As I said I was hoping for more cleanups but I think this still is a small improvement. I'm unsure where to put the `EnforcedCompression` and `EnforcedRequiredAcks`, and I'm wondering if keeping them in `TransactionLog` is the simplest option as moving them to `TransactionCoordinator` is not really helping.
--
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-18209 Cleanup `__transaction_state` config logic [kafka]
Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #18201:
URL: https://github.com/apache/kafka/pull/18201#discussion_r1939992923
##########
core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala:
##########
@@ -45,6 +46,15 @@ import org.apache.kafka.storage.internals.log.AppendOrigin
import scala.jdk.CollectionConverters._
import scala.collection.mutable
+object TransactionStateManager {
+ // enforce always using
+ // 1. cleanup policy = compact
+ // 2. compression = none
+ // 3. unclean leader election = disabled
+ // 4. required acks = -1 when writing
+ val EnforcedCompression: Compression = Compression.NONE
Review Comment:
I corrected it.
Is that what you wanted?
--
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-18209 Cleanup `__transaction_state` config logic [kafka]
Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on PR #18201:
URL: https://github.com/apache/kafka/pull/18201#issuecomment-2589979484
@mimaison Can you tell me if there's anything I can do to improve my 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
Re: [PR] KAFKA-18209 Cleanup `__transaction_state` config logic [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #18201:
URL: https://github.com/apache/kafka/pull/18201#discussion_r1939703891
##########
core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala:
##########
@@ -45,6 +46,15 @@ import org.apache.kafka.storage.internals.log.AppendOrigin
import scala.jdk.CollectionConverters._
import scala.collection.mutable
+object TransactionStateManager {
+ // enforce always using
+ // 1. cleanup policy = compact
+ // 2. compression = none
+ // 3. unclean leader election = disabled
+ // 4. required acks = -1 when writing
+ val EnforcedCompression: Compression = Compression.NONE
Review Comment:
We also want to use these fields in `transactionTopicConfigs()`
--
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-18209 Cleanup `__transaction_state` config logic [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18201:
URL: https://github.com/apache/kafka/pull/18201#issuecomment-2968934274
This PR is being marked as stale since it has not had any activity in 90 days. If you
would like to keep this PR alive, please leave a comment asking for a review. If the PR has
merge conflicts, update it with the latest from the base branch.
<p>
If you are having difficulty finding a reviewer, please reach out on the
[mailing list](https://kafka.apache.org/contact).
<p>
If this PR is no longer valid or desired, please feel free to close it. If no activity
occurs in the next 30 days, it will be automatically closed.
--
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-18209 Cleanup `__transaction_state` config logic [kafka]
Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #18201:
URL: https://github.com/apache/kafka/pull/18201#discussion_r1945312258
##########
core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala:
##########
@@ -400,12 +404,20 @@ class TransactionStateManager(brokerId: Int,
def validateTransactionTimeoutMs(txnTimeoutMs: Int): Boolean =
txnTimeoutMs <= config.transactionMaxTimeoutMs && txnTimeoutMs > 0
+ /**
+ * Enforce always using:
+ * <br>1. cleanup policy = compact
+ * <br>2. compression = none
+ * <br>3. unclean leader election = disabled
+ * <br>4. required acks = -1 when writing
Review Comment:
Fixed
--
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-18209 Cleanup `__transaction_state` config logic [kafka]
Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv closed pull request #18201: KAFKA-18209 Cleanup `__transaction_state` config logic
URL: https://github.com/apache/kafka/pull/18201
--
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-18209 Cleanup `__transaction_state` config logic [kafka]
Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #18201:
URL: https://github.com/apache/kafka/pull/18201#discussion_r1945320499
##########
core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala:
##########
@@ -45,6 +46,15 @@ import org.apache.kafka.storage.internals.log.AppendOrigin
import scala.jdk.CollectionConverters._
import scala.collection.mutable
+object TransactionStateManager {
+ // enforce always using
+ // 1. cleanup policy = compact
+ // 2. compression = none
+ // 3. unclean leader election = disabled
+ // 4. required acks = -1 when writing
+ val EnforcedCompression: Compression = Compression.NONE
Review Comment:
Since `TransactionStateManager#transactionTopicConfigs` is used only in `TransactionCoordinator`, it makes sense to store these values in `TransactionCoordinator`.
--
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-18209 Cleanup `__transaction_state` config logic [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18201:
URL: https://github.com/apache/kafka/pull/18201#issuecomment-2558842183
A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the `triage` label
should be removed to prevent this automation from happening again.
--
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-18209 Cleanup `__transaction_state` config logic [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #18201:
URL: https://github.com/apache/kafka/pull/18201#issuecomment-2594379744
A label of 'needs-attention' was automatically added to this PR in order to raise the
attention of the committers. Once this issue has been triaged, the `triage` label
should be removed to prevent this automation from happening again.
--
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-18209 Cleanup `__transaction_state` config logic [kafka]
Posted by "wernerdv (via GitHub)" <gi...@apache.org>.
wernerdv commented on code in PR #18201:
URL: https://github.com/apache/kafka/pull/18201#discussion_r1945310182
##########
core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala:
##########
@@ -37,14 +38,17 @@ import org.apache.kafka.common.{KafkaException, TopicPartition}
import org.apache.kafka.coordinator.transaction.{TransactionLogConfig, TransactionStateManagerConfig}
import org.apache.kafka.server.common.{RequestLocal, TransactionVersion}
import org.apache.kafka.server.config.ServerConfigs
-import org.apache.kafka.server.record.BrokerCompressionType
import org.apache.kafka.server.storage.log.FetchIsolation
import org.apache.kafka.server.util.Scheduler
import org.apache.kafka.storage.internals.log.AppendOrigin
import scala.jdk.CollectionConverters._
import scala.collection.mutable
+object TransactionStateManager {
+ val EnforcedCompression: Compression = Compression.NONE
+ private val EnforcedRequiredAcks: Short = -1.toShort
Review Comment:
Done
--
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