Hi, Raman, Thanks for the KIP. A couple of comments.
10. From the discussion of KIP-951, it seems that our convention is to always bump up the request version even for just adding tagged fields in the response. 11. "In case a new AdminClient is sending durationFilter (greater than 0) to an older broker, ListTransactionsRequest will fail to build at the client side. This will require some check to be made at ListTransactionsRequest.Builder.build(short version) method. A new AdminClient can still generate older version of ListTransactionsRequest when it sets durationFilter to 0." This seems weird. If a user specifies a durationFilter but the server doesn't support it, it seems that we should throw an exception to the user instead of silently changing durationFilter. Jun On Wed, Nov 29, 2023 at 5:05 PM Justine Olshan <jols...@confluent.io.invalid> wrote: > Hey Raman, > Thanks for the KIP! I think this will be super useful. > > Given https://issues.apache.org/jira/browse/KAFKA-15546 -- do you think it > would be useful to specify the duration of the completed transaction rather > than the time since the start in the describe output? > We would probably want to specify the transaction is completed as well to > differentiate from the response that does not have the tagged field. This > would more clearly resolve the ticket. > Maybe this was the plan but it wasn't clear from the KIP. > > Thanks, > Justine > > On Tue, Nov 28, 2023 at 12:38 PM Jason Gustafson > <ja...@confluent.io.invalid> > wrote: > > > Hey Raman, > > > > Thanks for the KIP! I think it makes sense. I agree that this becomes > > especially useful in the context of KIP-939 because transactions can last > > an indefinite amount of time, but it is useful even today. A large > cluster > > may have a very large number of ongoing transactions at any time, so > > providing a better way to filter by time will make the tools much more > > efficient for this common use case. > > > > I have just a couple small comments. > > > > 1. In `ListTransactionsOptions`, we use a long in the setter for the > > duration filter. Can we use `Duration` instead? > > 2. I think we need to expose `TransactionLastUpdateTimeMs` on > > `TransactionDescription` as well, right? > > > > Thanks, > > Jason > > > > On Tue, Nov 28, 2023 at 8:34 AM Kirk True <k...@kirktrue.pro> wrote: > > > > > Hi Raman, > > > > > > Thanks for the KIP! > > > > > > Questions/comments: > > > > > > 1. Is there a Jira created for this? The Jira link still points to > > > KAFKA-1. > > > 2. There's a minor typo in the ListTransactionsRequest documentation > for > > > the DurationFilter: trsanactions. > > > 3. Is the TransactionStartTimeMs return value in the > > > DescribeTransactionsResponse nullable? > > > 4. The API uses the general terminology of a "duration filter" but the > > > CLI uses the specific phrase "running longer than ms." These are both > > > referring to the same value, right? Can the naming of these two be more > > > consistent? > > > 5. What happens when a user runs the updated kafka-transactions.sh > > script > > > (using the new argument) against an older broker that doesn't support > the > > > new filter? Does the user get an error, a warning, or a silent ignoring > > of > > > the filter? > > > > > > Thanks, > > > Kirk > > > > > > On Wed, Nov 15, 2023, at 3:03 PM, Raman Verma wrote: > > > > Thanks Artem, > > > > > > > > I have made changes to the `Public Interfaces` and `Compatibility...` > > > > sections to incorporate your comment. > > > > > > > > On Mon, Nov 6, 2023 at 3:44 PM Raman Verma <raman.x.ve...@gmail.com> > > > wrote: > > > > > > > > > I would like to start a discussion on KIP-994 > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-994%3A+Minor+Enhancements+to+ListTransactions+and+DescribeTransactions+APIs > > > > > > > > > > > > > > > > > > > >
