Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
dajac commented on PR #16158: URL: https://github.com/apache/kafka/pull/16158#issuecomment-2158198985 Merged to trunk and to 3.8. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
dajac merged PR #16158: URL: https://github.com/apache/kafka/pull/16158 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
dajac commented on PR #16158: URL: https://github.com/apache/kafka/pull/16158#issuecomment-2157499078 CI does not seem to be working... -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
dajac commented on PR #16158: URL: https://github.com/apache/kafka/pull/16158#issuecomment-2157411292 Merged trunk to include https://github.com/apache/kafka/pull/16249. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
chia7712 commented on PR #16158: URL: https://github.com/apache/kafka/pull/16158#issuecomment-2156662574 I prefer to check all failed tests before merging. And the last commit of this PR does not have completed CI. That is why I suggest to rebase code to trigger QA 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
chia7712 commented on PR #16158: URL: https://github.com/apache/kafka/pull/16158#issuecomment-2156661854 @riedelmax #16249 fix the blocked tests. Without that fix, the CI will get timeout when running your 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
riedelmax commented on PR #16158: URL: https://github.com/apache/kafka/pull/16158#issuecomment-2156601592 @chia7712 would you mind to explain how #16249 effects this PR? if there are no conflicts with trunk i dont need to rebase or merge manually right? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
chia7712 commented on PR #16158: URL: https://github.com/apache/kafka/pull/16158#issuecomment-2156006198 @riedelmax please rebase code to have the fix #16249 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
riedelmax commented on code in PR #16158: URL: https://github.com/apache/kafka/pull/16158#discussion_r1631440593 ## core/src/test/scala/unit/kafka/server/ConsumerGroupDescribeRequestsTest.scala: ## @@ -116,6 +115,7 @@ class ConsumerGroupDescribeRequestsTest(cluster: ClusterInstance) extends GroupC val timeoutMs = 5 * 60 * 1000 val clientId = "client-id" val clientHost = "/127.0.0.1" +val authorizedOperationsInt = 328; // Integer representation of the authorized operations for this request 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
dajac commented on code in PR #16158: URL: https://github.com/apache/kafka/pull/16158#discussion_r1631388315 ## core/src/test/scala/unit/kafka/server/ConsumerGroupDescribeRequestsTest.scala: ## @@ -116,6 +115,7 @@ class ConsumerGroupDescribeRequestsTest(cluster: ClusterInstance) extends GroupC val timeoutMs = 5 * 60 * 1000 val clientId = "client-id" val clientHost = "/127.0.0.1" +val authorizedOperationsInt = 328; // Integer representation of the authorized operations for this request Review Comment: I wonder if we should replace `328` by something like ``` Utils.to32BitField( AclEntry.supportedOperations(ResourceType.CLUSTER).asScala .map(_.code.asInstanceOf[JByte]).asJava) ``` What do you think? We could also do it in the other place. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
dajac commented on code in PR #16158:
URL: https://github.com/apache/kafka/pull/16158#discussion_r1631285093
##
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##
@@ -7086,6 +7086,7 @@ class KafkaApisTest extends Logging {
def testConsumerGroupDescribe(): Unit = {
Review Comment:
Yeah, adding it to the existing integration test is fine for me.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
riedelmax commented on code in PR #16158:
URL: https://github.com/apache/kafka/pull/16158#discussion_r1631203974
##
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##
@@ -7086,6 +7086,7 @@ class KafkaApisTest extends Logging {
def testConsumerGroupDescribe(): Unit = {
Review Comment:
@dajac Do you think it is necessary to have two tests, one with
includeAuthorizedOperations and one without it?
I could at it to the IT, without having one IT with
includeAuthorizedOperations=false.
Its covered in the unit tests anyways and the option does not change the
behavior of the API
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
dajac commented on code in PR #16158:
URL: https://github.com/apache/kafka/pull/16158#discussion_r1631057079
##
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##
@@ -7107,11 +7108,12 @@ class KafkaApisTest extends Logging {
).asJava)
// Can't reuse the above list here because we would not test the
implementation in KafkaApis then
+val authorizedOperationsInt = if (includeAuthorizedOperations) 328 else
Int.MinValue;
val describedGroups = List(
new DescribedGroup().setGroupId(groupIds.get(0)),
new DescribedGroup().setGroupId(groupIds.get(1)),
new DescribedGroup().setGroupId(groupIds.get(2))
-).map(group => group.setAuthorizedOperations(328)) // Integer
representation of authorized operations for this request
+).map(group => group.setAuthorizedOperations(authorizedOperationsInt)) //
Integer representation of authorized operations for this request
Review Comment:
nit: Should we move this comment next to `328`?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
dajac commented on PR #16158: URL: https://github.com/apache/kafka/pull/16158#issuecomment-2154635171 @riedelmax There are a bunch of conflicts. Could you please fix them? Regarding `ConsumerGroupDescribeRequestsTest`, is it possible to extend it too? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
chia7712 commented on PR #16158: URL: https://github.com/apache/kafka/pull/16158#issuecomment-2146691340 > We have some integration tests in ConsumerGroupDescribeRequestTest thanks for this reminder. I neglect that before :( -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
riedelmax commented on PR #16158: URL: https://github.com/apache/kafka/pull/16158#issuecomment-2146678085 @chia7712 im not sure if I understand you correctlu. We have some integration tests in `ConsumerGroupDescribeRequestTest` -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
riedelmax commented on code in PR #16158:
URL: https://github.com/apache/kafka/pull/16158#discussion_r1625394359
##
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##
@@ -7086,6 +7086,7 @@ class KafkaApisTest extends Logging {
def testConsumerGroupDescribe(): Unit = {
Review Comment:
good idea
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
riedelmax commented on PR #16158: URL: https://github.com/apache/kafka/pull/16158#issuecomment-2146676215 @dajac thanks for checking so quickly. I will have time on friday to do the integration tet -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
dajac commented on code in PR #16158:
URL: https://github.com/apache/kafka/pull/16158#discussion_r1623901700
##
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##
@@ -7086,6 +7086,7 @@ class KafkaApisTest extends Logging {
def testConsumerGroupDescribe(): Unit = {
Review Comment:
Should we parameterize the tests to test both variants?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
chia7712 commented on PR #16158: URL: https://github.com/apache/kafka/pull/16158#issuecomment-2143262308 out of curiosity, do we have IT for that option? I grep code base and it seems the related ITs are running with old coordinator/protocol. For example: 1. https://github.com/apache/kafka/blob/fb566e48bf05d749f8db8da803a3570acf25bb11/core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala#L1213 2. https://github.com/apache/kafka/blob/fb566e48bf05d749f8db8da803a3570acf25bb11/core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala#L139 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
dajac commented on code in PR #16158:
URL: https://github.com/apache/kafka/pull/16158#discussion_r1622909655
##
core/src/main/scala/kafka/server/KafkaApis.scala:
##
@@ -3852,6 +3853,17 @@ class KafkaApis(val requestChannel: RequestChannel,
if (exception != null) {
requestHelper.sendMaybeThrottle(request,
consumerGroupDescribeRequest.getErrorResponse(exception))
} else {
+ if (request.header.apiVersion >= 3 && includeAuthorizedOperations) {
Review Comment:
I suppose that we don't need the version check because this API only has
version zero.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
dajac commented on PR #16158: URL: https://github.com/apache/kafka/pull/16158#issuecomment-2142879003 @riedelmax Thanks for the patch. Could you please extend unit and integration tests to cover this change? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] KAFKA-14509: [4/4] Handle includeAuthorizedOperations [kafka]
riedelmax opened a new pull request, #16158: URL: https://github.com/apache/kafka/pull/16158 Last PR for KAFKA-14509 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
