frankvicky opened a new pull request, #16250:
URL: https://github.com/apache/kafka/pull/16250
Currently, the return types of some methods in `KafkaAdminClient` use
immutable maps, but they should follow the convention of returning `HashMap`
like other methods.
### Committer Checklist
frankvicky commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2156027791
Hi @chia7712 , I will take 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 spec
frankvicky commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2156084937
Hi @chia7712, I have replace some `UnmodifiableMap` with `HashMap`, PTAL 😀
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to Git
chia7712 commented on code in PR #16250:
URL: https://github.com/apache/kafka/pull/16250#discussion_r1632066886
##
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##
@@ -3649,7 +3652,7 @@ public DeleteConsumerGroupOffsetsResult
deleteConsumerGroupOff
dajac commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2156186787
Out of curiosity, what’s the reason for not returning unmodifiable maps? I
think that we should actually ensure that the admin client does not return
modifiable maps.
--
This is an autom
chia7712 commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2156189050
> Out of curiosity, what’s the reason for not returning unmodifiable maps? I
think that we should actually ensure that the admin client does not return
modifiable maps.
I prefer t
frankvicky commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2156323628
Hi @chia7712, I have do a simple change based on your feedback, PTAL 🐧
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub
chia7712 commented on code in PR #16250:
URL: https://github.com/apache/kafka/pull/16250#discussion_r1632361233
##
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##
@@ -2760,9 +2760,12 @@ void handleFailure(Throwable throwable) {
}, now)
frankvicky commented on code in PR #16250:
URL: https://github.com/apache/kafka/pull/16250#discussion_r1632479332
##
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##
@@ -2760,9 +2760,12 @@ void handleFailure(Throwable throwable) {
}, no
frankvicky commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2157010773
Hi @chia7712, I have do a simple change based on your feedback, PTAL 😸
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHu
AndrewJSchofield commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2157670039
I don't quite understand this PR. Why are we replacing
`Collections.unmodifiableMap` with `HashMap`, but we are still happy to return
`Collectors.toMap` which states in the javad
chia7712 commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2157809066
> I don't quite understand this PR. Why are we replacing
Collections.unmodifiableMap with HashMap, but we are still happy to return
Collectors.toMap which states in the javadoc that "The
dajac commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2157893813
> > Out of curiosity, what’s the reason for not returning unmodifiable maps?
I think that we should actually ensure that the admin client does not return
modifiable maps.
>
> I prefe
AndrewJSchofield commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2157899046
Personally, I would prefer all returned maps to be unmodifiable, but there
is a small chance that moving from modifiable map to unmodifiable map would
cause an application to bre
chia7712 commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2158301859
> I suggest leaving unmodifiable maps where we have them today, and making
all new maps in the future unmodifiable.
That is most difficult part. We have no checks/rules for it, and
AndrewJSchofield commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2158354696
Works 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
dajac commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2158403995
Regarding
https://github.com/apache/kafka/commit/c01279b92acefd9135089588319910bac79bfd4c,
I think that `DescribeTopicPartitions` API in the admin client was not
released yet so we may be
chia7712 commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2158476566
> Regarding
https://github.com/apache/kafka/commit/c01279b92acefd9135089588319910bac79bfd4c,
I think that DescribeTopicPartitions API in the admin client was not released
yet so we may
frankvicky commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2158714543
IMHO, I agree that we should use an immutable map to ensure that users
cannot make any unintended modifications. However, to ensure that the original
design intent is protected(like we
chia7712 commented on code in PR #16250:
URL: https://github.com/apache/kafka/pull/16250#discussion_r1635327471
##
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##
@@ -2760,9 +2760,10 @@ void handleFailure(Throwable throwable) {
}, now)
frankvicky commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2162052602
Hi @chia7712 , I have make a change based on comments, PTAL 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use
chia7712 commented on code in PR #16250:
URL: https://github.com/apache/kafka/pull/16250#discussion_r1635784034
##
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##
@@ -2760,9 +2760,19 @@ void handleFailure(Throwable throwable) {
}, now)
frankvicky commented on code in PR #16250:
URL: https://github.com/apache/kafka/pull/16250#discussion_r1635795430
##
clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java:
##
@@ -2760,9 +2760,19 @@ void handleFailure(Throwable throwable) {
}, no
frankvicky commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2162268240
Hi @chia7712 , I have add comment and test case based on comment, PTAL 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitH
chia7712 commented on code in PR #16250:
URL: https://github.com/apache/kafka/pull/16250#discussion_r1637149134
##
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##
@@ -1988,6 +1989,15 @@ public void testDescribeClientMetricsConfigs() throws
Exc
frankvicky commented on code in PR #16250:
URL: https://github.com/apache/kafka/pull/16250#discussion_r1637320049
##
clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java:
##
@@ -1988,6 +1989,15 @@ public void testDescribeClientMetricsConfigs() throws
E
frankvicky commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2165115762
Hi @chia7712 , I have do a small change based on comment, PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use
chia7712 commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2167204001
@frankvicky please fix the build error
--
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
frankvicky commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2167221151
Hi @chia7712 , I have fixed the build error, PTAL
--
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
chia7712 merged PR #16250:
URL: https://github.com/apache/kafka/pull/16250
--
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
chia7712 commented on PR #16250:
URL: https://github.com/apache/kafka/pull/16250#issuecomment-2168308661
I update the topic to correspond to new target.
--
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
31 matches
Mail list logo
