Re: [DISCUSS] KIP-994: Minor Enhancements to ListTransactions and DescribeTransactions APIs
I have started a Vote on this KIP https://lists.apache.org/thread/yknx3bc4mk17bz2cpfr789lh8sx2lc39
Re: [DISCUSS] KIP-994: Minor Enhancements to ListTransactions and DescribeTransactions APIs
Thanks Jun, Justine, Jason, Kirk, I have addressed your comments.
Re: [DISCUSS] KIP-994: Minor Enhancements to ListTransactions and DescribeTransactions APIs
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 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 > > 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 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 > > > 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 > > > > > > > > > > > > > > > > > > > >
Re: [DISCUSS] KIP-994: Minor Enhancements to ListTransactions and DescribeTransactions APIs
Thanks Kirk, I have made the changes you mentioned. Regarding these questions 3. Is the TransactionStartTimeMs return value in the DescribeTransactionsResponse nullable? Broker can send a value less than zero but not null. 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? Given we are bumping the API version for ListTransactionsRequest, in this case the client will first try to build an API with an older version supported by the broker. We will throw an error at the client side when building an older version API with durationFilter. If the user does not specify durationFilter, AdminClient will be able to build the older version request that does not need durationFilter.
Re: [DISCUSS] KIP-994: Minor Enhancements to ListTransactions and DescribeTransactions APIs
Thanks Artem, I have made the changes to the KIP as specified. I think I will start a Vote on the KIP tomorrow.
Re: [DISCUSS] KIP-994: Minor Enhancements to ListTransactions and DescribeTransactions APIs
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 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 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 > > 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 > > > > > > > > > > > > > >
Re: [DISCUSS] KIP-994: Minor Enhancements to ListTransactions and DescribeTransactions APIs
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 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 > 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 > > > > > > > > >
Re: [DISCUSS] KIP-994: Minor Enhancements to ListTransactions and DescribeTransactions APIs
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 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 > > > > >
Re: [DISCUSS] KIP-994: Minor Enhancements to ListTransactions and DescribeTransactions APIs
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 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 > >
Re: [DISCUSS] KIP-994: Minor Enhancements to ListTransactions and DescribeTransactions APIs
Hi Raman, I see that you've updated the KIP. The content looks good to me. A couple nits on the format: - can you highlight which fields are new in the message? - can you add your original proposal of using a tagged field in ListTransactionsRequest to the list of rejected alternatives? -Artem On Tue, Nov 7, 2023 at 11:04 PM Andrew Schofield < andrew_schofield_j...@outlook.com> wrote: > Hi Artem, > I think you make a very good point. This also looks to me like it deserves > a version bump for the request. > > Andrew > > > On 8 Nov 2023, at 04:12, Artem Livshits > wrote: > > > > Hi Raman, > > > > Thank you for the KIP. I think using the tagged field > > in DescribeTransactionsResponse should be good -- if either the client or > > the server don't support it, it's not printed, which is reasonable > behavior. > > > > For the ListTransactionsRequest, though, I think using the tagged field > > could lead to a subtle compatibility issue if a new client is used with > old > > server: the client could specify the DurationFilter, but the old server > > would ignore it and list all transactions instead, which could be > > misleading or potentially even dangerous if the results are used in a > > script for some automation. I think a more desirable behavior would be > to > > fail if the server doesn't support the new filter, which we should be > able > > to achieve if we bump version of the ListTransactionsRequest and add > > DurationFilter as a regular field. > > > > -Artem > > > > On Tue, Nov 7, 2023 at 2:20 AM Raman Verma > 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 > >> > >
Re: [DISCUSS] KIP-994: Minor Enhancements to ListTransactions and DescribeTransactions APIs
Hi Artem, I think you make a very good point. This also looks to me like it deserves a version bump for the request. Andrew > On 8 Nov 2023, at 04:12, Artem Livshits > wrote: > > Hi Raman, > > Thank you for the KIP. I think using the tagged field > in DescribeTransactionsResponse should be good -- if either the client or > the server don't support it, it's not printed, which is reasonable behavior. > > For the ListTransactionsRequest, though, I think using the tagged field > could lead to a subtle compatibility issue if a new client is used with old > server: the client could specify the DurationFilter, but the old server > would ignore it and list all transactions instead, which could be > misleading or potentially even dangerous if the results are used in a > script for some automation. I think a more desirable behavior would be to > fail if the server doesn't support the new filter, which we should be able > to achieve if we bump version of the ListTransactionsRequest and add > DurationFilter as a regular field. > > -Artem > > On Tue, Nov 7, 2023 at 2:20 AM Raman Verma 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 >>
Re: [DISCUSS] KIP-994: Minor Enhancements to ListTransactions and DescribeTransactions APIs
Hi Raman, Thank you for the KIP. I think using the tagged field in DescribeTransactionsResponse should be good -- if either the client or the server don't support it, it's not printed, which is reasonable behavior. For the ListTransactionsRequest, though, I think using the tagged field could lead to a subtle compatibility issue if a new client is used with old server: the client could specify the DurationFilter, but the old server would ignore it and list all transactions instead, which could be misleading or potentially even dangerous if the results are used in a script for some automation. I think a more desirable behavior would be to fail if the server doesn't support the new filter, which we should be able to achieve if we bump version of the ListTransactionsRequest and add DurationFilter as a regular field. -Artem On Tue, Nov 7, 2023 at 2:20 AM Raman Verma 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 >
