Re: [PR] KAFKA-15942: Implement ConsumerInterceptor [kafka]

2023-12-23 Thread via GitHub


Joker-5 commented on PR #14963:
URL: https://github.com/apache/kafka/pull/14963#issuecomment-1868456838

   > @Joker-5 , thanks for the update. There are some missing pieces in this PR 
from what I can tell. Having said that, I see another PR created for this 
[here](https://github.com/apache/kafka/pull/15000). You might want to check 
with @lucasbru about this.
   
   Thanks so much for the review! 
   I had checked with @lucasbru. We will merge @lucasbru's PR which has the two 
changes and @lucasbru will add me in a Co-authored-by tag.


-- 
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-15942: Implement ConsumerInterceptor [kafka]

2023-12-23 Thread via GitHub


Joker-5 commented on PR #14963:
URL: https://github.com/apache/kafka/pull/14963#issuecomment-1868454660

   > Hey @Joker-5, I took the ticket since your original PR seemed to only 
change the legacy consumer, so I thought it was just linked to the wrong ticket.
   > 
   > I think there are some things missing here
   > 
   > * enable unit / integration tests
   > * the way you implemented it, I think the interceptors will run as part of 
the background thread, but I think they should not interfere with the 
background thread and run as part of the application thread instead.
   > 
   > How about we merge my PR which has the two changes and I add you in a 
`Co-authored-by` tag? Sorry again for the confusion.
   
   I understand, this is the second PR which i committed to Kafka. And I had 
learned a lot from your PR, so just do it.


-- 
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



[jira] [Commented] (KAFKA-15341) Enabling TS for a topic during rolling restart causes problems

2023-12-23 Thread Phuc Hong Tran (Jira)


[ 
https://issues.apache.org/jira/browse/KAFKA-15341?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17800124#comment-17800124
 ] 

Phuc Hong Tran commented on KAFKA-15341:


hi [~divijvaidya], if you have time, can you help me reviews the comments I 
wrote above? Thanks

> Enabling TS for a topic during rolling restart causes problems
> --
>
> Key: KAFKA-15341
> URL: https://issues.apache.org/jira/browse/KAFKA-15341
> Project: Kafka
>  Issue Type: Bug
>Reporter: Divij Vaidya
>Assignee: Phuc Hong Tran
>Priority: Major
>  Labels: KIP-405
> Fix For: 3.7.0
>
>
> When we are in a rolling restart to enable TS at system level, some brokers 
> have TS enabled on them and some don't. We send an alter config call to 
> enable TS for a topic, it hits a broker which has TS enabled, this broker 
> forwards it to the controller and controller will send the config update to 
> all brokers. When another broker which doesn't have TS enabled (because it 
> hasn't undergone the restart yet) gets this config change, it "should" fail 
> to apply it. But failing now is too late since alterConfig has already 
> succeeded since controller->broker config propagation is done async.
> With this JIRA, we want to have controller check if TS is enabled on all 
> brokers before applying alter config to turn on TS for a topic.
> Context: https://github.com/apache/kafka/pull/14176#discussion_r1291265129



--
This message was sent by Atlassian Jira
(v8.20.10#820010)


[jira] [Commented] (KAFKA-15749) KRaft support in KafkaMetricReporterClusterIdTest

2023-12-23 Thread Anish Lukkireddy (Jira)


[ 
https://issues.apache.org/jira/browse/KAFKA-15749?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17800091#comment-17800091
 ] 

Anish Lukkireddy commented on KAFKA-15749:
--

 can this be assinged to me

> KRaft support in KafkaMetricReporterClusterIdTest
> -
>
> Key: KAFKA-15749
> URL: https://issues.apache.org/jira/browse/KAFKA-15749
> Project: Kafka
>  Issue Type: Task
>  Components: core
>Reporter: Sameer Tejani
>Priority: Minor
>  Labels: kraft, kraft-test, newbie
>
> The following tests in KafkaMetricReporterClusterIdTest in 
> core/src/test/scala/unit/kafka/server/KafkaMetricReporterClusterIdTest.scala 
> need to be updated to support KRaft
> 96 : def testClusterIdPresent(): Unit = {
> Scanned 119 lines. Found 0 KRaft tests out of 1 tests



--
This message was sent by Atlassian Jira
(v8.20.10#820010)


[jira] [Commented] (KAFKA-15983) Kafka-acls should return authorization already done if repeating work is issued

2023-12-23 Thread Anish Lukkireddy (Jira)


[ 
https://issues.apache.org/jira/browse/KAFKA-15983?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17800088#comment-17800088
 ] 

Anish Lukkireddy commented on KAFKA-15983:
--

 can I someone assigned this ticket to me

> Kafka-acls should return authorization already done if repeating work is 
> issued
> ---
>
> Key: KAFKA-15983
> URL: https://issues.apache.org/jira/browse/KAFKA-15983
> Project: Kafka
>  Issue Type: Improvement
>  Components: security
>Affects Versions: 3.6.0
>Reporter: Chen He
>Priority: Minor
>  Labels: newbie
>
> kafka-acls.sh cmd will always issue normal operation for a cmd if customer 
> already authorized a user. It should reports something like "user {} already 
> authorized with {} resources" instead of do it again and again. 



--
This message was sent by Atlassian Jira
(v8.20.10#820010)


Re: [PR] KAFKA-9337 simplify mm2 initial configs [kafka]

2023-12-23 Thread via GitHub


Gakhramanzode commented on PR #7872:
URL: https://github.com/apache/kafka/pull/7872#issuecomment-1868294680

   Hello Kafka Developers,
   
   I hope this message finds you well. I am currently using Kafka Mirror Maker 
and have set up a monitoring alert with Prometheus Alertmanager. Here is the 
alert rule I'm using:
   ```yaml
   - alert: NodeKafkaMirrorMakerServiceDown
 expr: node_systemd_unit_state{name="kafka-mirror-maker.service", 
state="active"} == 0
 for: 1m
 labels:
   severity: error
   project: my-project
 annotations:
   summary: "Kafka Mirror Maker service is down"
   instance: "{{ $labels.instance }}"
   value: "{{ $value }}"
   ```
   I would like to know if this is considered a good practice for monitoring 
the Kafka Mirror Maker service. Any feedback or suggestions would be greatly 
appreciated.
   
   Thank you for your time and assistance.
   
   Best regards, Asker
   
   @cryptoe @ryannedolan @mimaison @rpozarickij @halorgium 
   


-- 
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-16046: fix stream-stream-join store types [kafka]

2023-12-23 Thread via GitHub


mjsax commented on code in PR #15061:
URL: https://github.com/apache/kafka/pull/15061#discussion_r1435548304


##
streams/src/main/java/org/apache/kafka/streams/state/DslWindowParams.java:
##
@@ -44,15 +45,18 @@ public class DslWindowParams {
  * caching and means that null values will be 
ignored.
  * @param emitStrategy defines how to emit results
  * @param isSlidingWindow  whether the requested store is a sliding window
+ * @param isTimestampedwhether the requested store should be 
timestamped (see {@link TimestampedWindowStore}
  */
 public DslWindowParams(
 final String name,
 final Duration retentionPeriod,
 final Duration windowSize,
 final boolean retainDuplicates,
 final EmitStrategy emitStrategy,
-final boolean isSlidingWindow
+final boolean isSlidingWindow,
+final boolean isTimestamped

Review Comment:
   Looks like a public API change -- did we update the KIP accordingly?



-- 
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



[jira] [Updated] (KAFKA-16046) Stream Stream Joins fail after restoration with deserialization exceptions

2023-12-23 Thread Matthias J. Sax (Jira)


 [ 
https://issues.apache.org/jira/browse/KAFKA-16046?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Matthias J. Sax updated KAFKA-16046:

Fix Version/s: 3.7.0

> Stream Stream Joins fail after restoration with deserialization exceptions
> --
>
> Key: KAFKA-16046
> URL: https://issues.apache.org/jira/browse/KAFKA-16046
> Project: Kafka
>  Issue Type: Bug
>  Components: streams
>Affects Versions: 3.7.0
>Reporter: Almog Gavra
>Assignee: Almog Gavra
>Priority: Blocker
>  Labels: streams
> Fix For: 3.7.0
>
>
> Before KIP-954, the `KStreamImplJoin` class would always create 
> non-timestamped persistent windowed stores. After that KIP, the default was 
> changed to create timestamped stores. This wasn't compatible because, during 
> restoration, timestamped stores have their changelog values transformed to 
> prepend the timestamp to the value. This caused serialization errors when 
> trying to read from the store because the deserializers did not expect the 
> timestamp to be prepended.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)


Re: [PR] KAFKA-16046: fix stream-stream-join store types [kafka]

2023-12-23 Thread via GitHub


mjsax commented on code in PR #15061:
URL: https://github.com/apache/kafka/pull/15061#discussion_r1435548304


##
streams/src/main/java/org/apache/kafka/streams/state/DslWindowParams.java:
##
@@ -44,15 +45,18 @@ public class DslWindowParams {
  * caching and means that null values will be 
ignored.
  * @param emitStrategy defines how to emit results
  * @param isSlidingWindow  whether the requested store is a sliding window
+ * @param isTimestampedwhether the requested store should be 
timestamped (see {@link TimestampedWindowStore}
  */
 public DslWindowParams(
 final String name,
 final Duration retentionPeriod,
 final Duration windowSize,
 final boolean retainDuplicates,
 final EmitStrategy emitStrategy,
-final boolean isSlidingWindow
+final boolean isSlidingWindow,
+final boolean isTimestamped

Review Comment:
   Looks like a public API change -- did we update the KIP accordingly?



-- 
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-14588 ConfigType moved to server-common [kafka]

2023-12-23 Thread via GitHub


nizhikov commented on PR #14867:
URL: https://github.com/apache/kafka/pull/14867#issuecomment-1868235889

   @mimaison Thanks you very musch for taking your time to review and merge PR 
just before holidays :).
   Happy New Year!


-- 
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