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/12/18 06:36:13 UTC

[GitHub] [kafka] dengziming opened a new pull request #9766: KAFKA-10864: Convert end txn marker schema to use auto-generated protocol

dengziming opened a new pull request #9766:
URL: https://github.com/apache/kafka/pull/9766


   *More detailed description of your change*
   1. Convert end txn marker schema to use auto-generated protocol `EndTxnMarker`
   2. substitute `CURRENT_END_TXN_MARKER_VALUE_SIZE` with an `endTnxMarkerValueSize` method since the size is accumulated from `EndTxnMarker`.
   3. add buffer to `EndTransactionMarker` to avoid twice compute from `serializeValue` and `endTnxMarkerValueSize`.
   
   *Summary of testing strategy (including rationale)*
   A unit test to verify serialize and deserialize
   
   ### 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] abbccdda commented on a change in pull request #9766: KAFKA-10864: Convert end txn marker schema to use auto-generated protocol

Posted by GitBox <gi...@apache.org>.
abbccdda commented on a change in pull request #9766:
URL: https://github.com/apache/kafka/pull/9766#discussion_r545985013



##########
File path: clients/src/main/java/org/apache/kafka/common/record/MemoryRecords.java
##########
@@ -618,8 +618,7 @@ public static MemoryRecords withEndTransactionMarker(long timestamp, long produc
     public static MemoryRecords withEndTransactionMarker(long initialOffset, long timestamp, int partitionLeaderEpoch,
                                                          long producerId, short producerEpoch,
                                                          EndTransactionMarker marker) {
-        int endTxnMarkerBatchSize = DefaultRecordBatch.RECORD_BATCH_OVERHEAD +
-                EndTransactionMarker.CURRENT_END_TXN_SCHEMA_RECORD_SIZE;
+        int endTxnMarkerBatchSize = DefaultRecordBatch.RECORD_BATCH_OVERHEAD + marker.endTnxMarkerValueSize();

Review comment:
       s/endTnxMarkerValueSize/endTxnMarkerValueSize

##########
File path: clients/src/main/resources/common/message/EndTxnMarker.json
##########
@@ -0,0 +1,23 @@
+// 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.
+
+{
+  "type": "data",
+  "name": "EndTxnMarker",
+  "validVersions": "0",
+  "fields": [
+    { "name": "CoordinatorEpoch", "type": "int32", "versions": "0+"}

Review comment:
       Add a comment for the field. Also do you think we need flexible version support 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] dengziming commented on pull request #9766: KAFKA-10864: Convert end txn marker schema to use auto-generated protocol

Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #9766:
URL: https://github.com/apache/kafka/pull/9766#issuecomment-748414238


   > Thanks for the PR, one high level question I have is that whether the new auto generated protocol format is exactly the same as old hard-coded schema format, since this field is used inter-broker. Could you write a test to prove that they could translate between each other?
   
   Thank you, I tested that the 2 schemas could translate between each other locally, now I add the code to `EndTransactionMarkerTest`.


----------------------------------------------------------------
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] dengziming commented on a change in pull request #9766: KAFKA-10864: Convert end txn marker schema to use auto-generated protocol

Posted by GitBox <gi...@apache.org>.
dengziming commented on a change in pull request #9766:
URL: https://github.com/apache/kafka/pull/9766#discussion_r546185834



##########
File path: clients/src/main/resources/common/message/EndTxnMarker.json
##########
@@ -0,0 +1,23 @@
+// 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.
+
+{
+  "type": "data",
+  "name": "EndTxnMarker",
+  "validVersions": "0",
+  "fields": [
+    { "name": "CoordinatorEpoch", "type": "int32", "versions": "0+"}

Review comment:
       Firstly I tried to use flexible version here, but it's not the same as the old hard-coded schema format. Could we bump the version and support flexible version from version 1 ?




----------------------------------------------------------------
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] dengziming commented on pull request #9766: KAFKA-10864: Convert end txn marker schema to use auto-generated protocol

Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #9766:
URL: https://github.com/apache/kafka/pull/9766#issuecomment-762235778


   ping @abbccdda , Also ping @chia7712 to have a look.


----------------------------------------------------------------
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