kirktrue commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-2400959059
@Phuc-Hong-Tran—I wanted to make sure that you weren't held up waiting for
input from others. After the number of review comments gets to a certain size,
it can be hard to tell where we
AndrewJSchofield commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1740038808
##
clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java:
##
@@ -141,6 +141,16 @@ public synchronized void subscribe(Pattern pattern) {
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1739981694
##
clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java:
##
@@ -141,6 +141,16 @@ public synchronized void subscribe(Pattern pattern) {
kirktrue commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1732961237
##
clients/src/main/java/org/apache/kafka/clients/consumer/SubscriptionPattern.java:
##
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1729748670
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java:
##
@@ -84,6 +85,9 @@ private enum SubscriptionType {
/* the pat
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1729747540
##
clients/src/main/resources/common/message/ConsumerGroupHeartbeatRequest.json:
##
@@ -13,6 +13,8 @@
// See the License for the specific language governing per
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1729747540
##
clients/src/main/resources/common/message/ConsumerGroupHeartbeatRequest.json:
##
@@ -13,6 +13,8 @@
// See the License for the specific language governing per
AndrewJSchofield commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1687645000
##
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java:
##
@@ -754,6 +752,49 @@ public void subscribe(Pattern pattern) {
delega
github-actions[bot] commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-2164295049
This PR is being marked as stale since it has not had any activity in 90
days. If you would like to keep this PR alive, please ask a committer for
review. If the PR has merge
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1993206506
Regex validity check will be included in the next pull request, I'll try to
get it done by this weekend.
--
This is an automated message from the Apache Git Service.
To respond t
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1519707860
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##
@@ -1730,6 +1744,21 @@ private void subscribeInternal(Pattern pa
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1517714052
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java:
##
@@ -84,6 +85,9 @@ private enum SubscriptionType {
/* the pat
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1518849655
##
clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java:
##
@@ -494,6 +501,14 @@ public void testSubscriptionOnEmptyPattern(GroupProtoc
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1518786653
##
clients/src/main/resources/common/message/ConsumerGroupHeartbeatRequest.json:
##
@@ -35,6 +38,8 @@
"about": "-1 if it didn't change since the last hear
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1517714052
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java:
##
@@ -84,6 +85,9 @@ private enum SubscriptionType {
/* the pat
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1517646778
##
clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java:
##
@@ -494,6 +501,14 @@ public void testSubscriptionOnEmptyPattern(GroupProtoc
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1517636440
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java:
##
@@ -84,6 +85,9 @@ private enum SubscriptionType {
/* the pat
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1513877668
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java:
##
@@ -84,6 +85,9 @@ private enum SubscriptionType {
/* the pat
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1513877668
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java:
##
@@ -84,6 +85,9 @@ private enum SubscriptionType {
/* the pat
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1511957999
##
clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java:
##
@@ -494,6 +501,14 @@ public void testSubscriptionOnEmptyPattern(GroupProtoc
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1510987276
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/LegacyKafkaConsumer.java:
##
@@ -495,6 +496,16 @@ public void subscribe(Pattern pattern) {
lianetm commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1961610927
Regarding the validation of the regex, I lean towards having a new error,
like @dajac suggested. Just to clearly tell the user that it is using an
invalid regex, without overcomplicating
cadonna commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1495579840
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##
@@ -1730,6 +1744,21 @@ private void subscribeInternal(Pattern pattern,
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1953901380
@cadonna I'll see if there is a way to go around with not using the Google
library to check regex validity (finger-crossed!)
--
This is an automated message from the Apache Git S
cadonna commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1953893744
> > > I was wondering whether we should introduce a new error code to signal
to the user that the regular expression is invalid. Otherwise, we would have to
use the invalid request except
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1953879987
> > I was wondering whether we should introduce a new error code to signal
to the user that the regular expression is invalid. Otherwise, we would have to
use the invalid request e
cadonna commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1953872828
> I was wondering whether we should introduce a new error code to signal to
the user that the regular expression is invalid. Otherwise, we would have to
use the invalid request exception
cadonna commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1953815656
> > > @cadonna @lianetm, since we're supporting for both subscribe method
using java.util.regex.Pattern and SubscriptionPattern, I think we should throw
a illegal heartbeat exeption when
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1952492560
> > > @cadonna @lianetm, since we're supporting for both subscribe method
using java.util.regex.Pattern and SubscriptionPattern, I think we should throw
a illegal heartbeat exeptio
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1492453933
##
clients/src/main/resources/common/message/ConsumerGroupHeartbeatRequest.json:
##
@@ -35,6 +35,8 @@
"about": "-1 if it didn't change since the last hear
dajac commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1948347606
I was wondering whether we should introduce a new error code to signal to
the user that the regular expression is invalid. Otherwise, we would have to
use the invalid request exception and
dajac commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1492422212
##
clients/src/main/resources/common/message/ConsumerGroupHeartbeatRequest.json:
##
@@ -35,6 +35,8 @@
"about": "-1 if it didn't change since the last heartbeat; th
dajac commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1948333168
> > @cadonna @lianetm, since we're supporting for both subscribe method
using java.util.regex.Pattern and SubscriptionPattern, I think we should throw
a illegal heartbeat exeption when user
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1941295224
> > @cadonna @lianetm, since we're supporting for both subscribe method
using java.util.regex.Pattern and SubscriptionPattern, I think we should throw
a illegal heartbeat exeption
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1941284769
@cadonna, sorry for the delay. I'll push the changes tomorrow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1941283018
@lianetm thanks for the reply. I was more wondering about the testing
strategy of the new subscribe(SubscriptionPattern) method when it comes to
receiving the assignment, since tha
cadonna commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1941278766
@Phuc-Hong-Tran Could you please implement the changes that I requested so
that we can move on?
--
This is an automated message from the Apache Git Service.
To respond to the message, p
cadonna commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1941264974
> I could be missing something, but I would say we don't need any changes
for the part where the client receives the assignment from the broker after
subscribing to a regex. It should be
cadonna commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1941258118
> @cadonna @lianetm, since we're supporting for both subscribe method using
java.util.regex.Pattern and SubscriptionPattern, I think we should throw a
illegal heartbeat exeption when user
lianetm commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1936141808
@Phuc-Hong-Tran regarding this:
> Just for clarification, when we were talking about "implement and test
everything up to the point where the field is populated", does that mean we'
lianetm commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1936091369
Hey @Phuc-Hong-Tran , regarding the mixed usage of subscribe with `Pattern`
and with `SubscriptionPattern`, my opinion is that it is something we should
live with to provide a smooth tran
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1930102760
@cadonna Just for clarification, when we were talking about "implement and
test everything up to the point where the field is populated", does that mean
we're not gonna implement a
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1929317343
@cadonna @lianetm, since we're supporting for both subscribe method using
java.util.regex.Pattern and SubscriptionPattern, I think we should throw a
illegal heartbeat exeption when
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1479598776
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java:
##
@@ -84,6 +85,9 @@ private enum SubscriptionType {
/* the pat
Phuc-Hong-Tran closed pull request #15188: KAFKA-15561: Client support for new
SubscriptionPattern based subscription
URL: https://github.com/apache/kafka/pull/15188
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL
cadonna commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1918921920
@dajac If you feel more comfortable, we could implement and test everything
up to the point where the field is populated. We would then not populate the
field so that you do not need to a
dajac commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1918915394
@Phuc-Hong-Tran Yep, that's 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 specif
dajac commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1918915086
@cadonna Yes. We could for instance commit the RPC and then work
independently on the client and the server. My only concern is that we usually
discover issues while working on the server s
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1918907960
@dajac, when we're talking about the RPC, do we mean the field for the regex
in ConsumerGroupHeartbeatRequest.json?
--
This is an automated message from the Apache Git Service.
T
cadonna commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1918901251
@dajac OK, but we can implement and unit test everything up to the RPC,
right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to G
dajac commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1918899056
In my opinion, we should merge the client side only after the server side is
implemented. The reason is that we need to change the RPC (this is actually
missing in this PR) and this should
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1918895644
I understand, will get back to speed on this one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL ab
cadonna commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1918887578
@Phuc-Hong-Tran
> though isn't it this one aiming for 3.8 release?
Yes, but we have time-based releases in Apache Kafka. That means that the
deadline for the release will be
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1918876275
@cadonna thanks for the comment, I can still finish one as the deadline
required, though isn't it this one aiming for 3.8 release? Also the logic on
the broker is not implemented y
cadonna commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1918874061
> - This feature is not yet implemented on the broker, so
t[here](https://github.com/apache/kafka/blob/055ff2b831193f5935f9efc2f7809f853f63de5f/clients/src/main/java/org/apache/kafka/clien
cadonna commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1472581740
##
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java:
##
@@ -753,6 +753,10 @@ public void subscribe(Pattern pattern,
ConsumerRebalanceListener
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1454760385
##
clients/src/main/java/org/apache/kafka/clients/consumer/SubscriptionPattern.java:
##
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF)
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1454761116
##
clients/src/test/java/org/apache/kafka/clients/consumer/internals/OffsetsRequestManagerTest.java:
##
@@ -86,7 +86,6 @@
import static org.mockito.Mockito.when
Phuc-Hong-Tran commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1454760385
##
clients/src/main/java/org/apache/kafka/clients/consumer/SubscriptionPattern.java:
##
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF)
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1894635327
Thanks @lianetm
--
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 commen
lianetm commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1894569927
This is the task to closely follow
https://issues.apache.org/jira/browse/KAFKA-14517, where the broker will
support the new regex.
--
This is an automated message from the Apache Git S
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1894475663
@lianetm, thanks for the comments, I will make sure to address those points
in my next PR.
Regarding your point about passing the regex for HeartbeatRequestManager, I
origni
lianetm commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1453968041
##
clients/src/test/java/org/apache/kafka/clients/consumer/internals/OffsetsRequestManagerTest.java:
##
@@ -86,7 +86,6 @@
import static org.mockito.Mockito.when;
pu
lianetm commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1453966464
##
clients/src/main/java/org/apache/kafka/clients/consumer/SubscriptionPattern.java:
##
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under
lianetm commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1453958313
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/SubscriptionState.java:
##
@@ -84,6 +85,9 @@ private enum SubscriptionType {
/* the pattern us
lianetm commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1453953690
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/LegacyKafkaConsumer.java:
##
@@ -494,6 +495,16 @@ public void subscribe(Pattern pattern) {
lianetm commented on code in PR #15188:
URL: https://github.com/apache/kafka/pull/15188#discussion_r1453945617
##
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java:
##
@@ -753,6 +753,10 @@ public void subscribe(Pattern pattern,
ConsumerRebalanceListener
Phuc-Hong-Tran commented on PR #15188:
URL: https://github.com/apache/kafka/pull/15188#issuecomment-1890427403
@lianetm, PTAL, thanks in advance.
--
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 t
Phuc-Hong-Tran opened a new pull request, #15188:
URL: https://github.com/apache/kafka/pull/15188
Change:
1. Implement methods in AsyncKafkaConsumer that accept SubscriptionPattern
to subscribe to topic(s).
2. Pass on the subscriptionPattern to SubscriptionState to use once server
sup
69 matches
Mail list logo
