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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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;
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
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
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,
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
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
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
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
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
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
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
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
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
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
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.
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
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
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
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
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
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,
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
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
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
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
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
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
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;
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
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,
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
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
60 matches
Mail list logo
