KAFKA-1683: persisting session information in Requests by gwenshap · Pull Request #155 · apache/kafka

gwenshap · 2015-08-21T01:48:02Z

No description provided.

asfbot · 2015-08-21T01:54:10Z

kafka-trunk-git-pr #184 FAILURE
Looks like there's a problem with this pull request

harshach · 2015-08-21T01:55:06Z

should we log a warning here

yeah, since we are caching the Principal in the authenticator, warning here makes sense.

btw. this is pretty weird - the session and the tests are on the server side, but since we are using the client network layer, you need to have log4j.logger.org.apache.kafka=WARN to see this warning.

Just for my benefit, what would this warning indicate to a Kafka user? Can they do something about it?

This will indicate that even though they are using SSL, it is SSL without authentication (just encryption). It could be intentional, but could also be a misconfiguration (such as misplaced key files), hence warning.

Understood, thanks.

harshach · 2015-08-21T01:57:08Z

+1 apart minor logging req.

asfbot · 2015-08-21T02:14:23Z

kafka-trunk-git-pr #185 SUCCESS
This pull request looks good

Parth-Brahmbhatt · 2015-08-21T02:23:49Z

+1.

asfbot · 2015-08-21T02:54:47Z

kafka-trunk-git-pr #188 SUCCESS
This pull request looks good

ijuma · 2015-08-21T07:56:51Z

Normally, one would use Option[Session] instead of a nullable one unless efficiency is of particular importance in that use-case. Is this one of those cases?

Nope :) Actually, since we always have a Session, I don't think Option is needed.
This was left from previous iteration.

Oh, even better.

ijuma · 2015-08-21T08:00:26Z

Thanks @gwenshap, I left some minor comments. One question I had was: do we need a test that uses the Session in some way or it's OK to leave as is because @Parth-Brahmbhatt's authorizer work will exercise it anyway?

eribeiro · 2015-08-21T10:49:38Z

Excuse me if my question is really naive/stupid, but there's a need to return a new anonymous object each time this function is called? I mean, KafkaPrincipal is a placeholder, name is final, and the principal is anonymous, so why not have:

public class KafkaPrincipal implements Principal { private final String name; public static final KafkaPrincipal ANONYMOUS = new KafkaPrincipal("ANONYMOUS"); (...)

Besides saving on object creation (kinda of Flyweight pattern) and GC, we could replace the anonymousPrincipal() method by KafkaPrincipal.ANONYMOUS. Does it make sense?

yeah, my bad. It was late :)

No need to excuse. I do this one myself all the time, lol. 👯

gwenshap · 2015-08-21T17:50:05Z

I added a test to SocketServerTest class. Do you think additional tests will help?

ijuma · 2015-08-21T17:59:16Z

@gwenshap Regarding the test, my bad, I hadn't seen that it gets the principal from the session.

asfbot · 2015-08-21T19:04:47Z

kafka-trunk-git-pr #190 FAILURE
Looks like there's a problem with this pull request

junrao · 2015-08-23T18:58:22Z

It will be useful to include the principal and the security type when logging the request in the requestLogger. Also, in the request log, instead of logging connectionId, we should log session.host instead.

I added the principal and security protocol to the logging. I left connectionId in place - currently it contains the host:port pairs of both sides of the connection and is therefore very informative. I'd like to keep this level of detail.

asfbot · 2015-08-24T20:37:33Z

kafka-trunk-git-pr #205 SUCCESS
This pull request looks good

junrao · 2015-08-26T14:16:53Z

To be consistent with the logging in KafkaApis, securityProtocol should be "security protocol"? Ditto for the logging below.

Even though the same logging line includes totalTime, requestQueueTime and so on?

Yeah, I tried to match the existing style here. I'll modify the extra details in KafkaApis to match.

Here's what we have after my change, nice and consistent :)

[2015-08-26 11:33:06,280] TRACE [KafkaApi-0] Handling request:Name: TopicMetadataRequest; Version: 0; CorrelationId: 0; ClientId: console-producer; Topics: t1 from connection 127.0.0.1:9092-127.0.0.1:64559;securityProtocol:PLAINTEXT,principal:ANONYMOUS (kafka.server.KafkaApis)

[2015-08-26 11:33:06,280] TRACE Completed request:Name: TopicMetadataRequest; Version: 0; CorrelationId: 0; ClientId: console-producer from connection 127.0.0.1:9092-127.0.0.1:64559;totalTime:1,requestQueueTime:1,localTime:0,remoteTime:0,responseQueueTime:0,sendTime:0,securityProtocol:PLAINTEXT,principal:ANONYMOUS (kafka.request.logger)

junrao · 2015-08-26T14:17:45Z

Thanks for the patch. Just a minor comment above. Otherwise, LGTM.

gwenshap · 2015-08-26T18:45:29Z

Since Jun LGTMed this and I addressed his minor comment, I'll go ahead and merge this.

Parth-Brahmbhatt · 2015-08-26T18:45:50Z

One question that I just remembered from Authorizer discussion, should the session have subject instead of Principal ?

gwenshap · 2015-08-26T18:48:16Z

Good point @Parth-Brahmbhatt. I totally forgot about that :)

Can you explain in which scenarios will this matter? Also, right now the SSL code exposes a Principal but not a Subject - do we need to change that too?

asfbot · 2015-08-26T18:55:08Z

kafka-trunk-git-pr #228 SUCCESS
This pull request looks good

ijuma · 2015-08-26T18:59:14Z

Would it make sense to merge this and iterate if needed in the authorizer PR?

Parth-Brahmbhatt · 2015-08-26T19:01:29Z

The Subject is kind of a wrapper around Principals and can support more use cases. Subject has other things like public/private Credentials + it can have a list of principals instead of single principal. These things might be helpful in future when we support authentication schemes, For Example sasl with impersonation is one example where we may have 2 principals, the user being impersonated and the impersonating user.

I guess we can live it as is for now and once we encounter a real use case we can change the type to subject.

I agree with @ijuma lets merge this in and we can discuss this later.

Modified the replication test to check for duplicates if idempotence is enabled.

TICKET = N/A LI_DESCRIPTION = The test testIllegalRequiredAcks has been flaky in the CI workflow. Yet I'm not able to reproduce the failure on my local machine. Thus I'm temporarily ignoring the test to unblock further development. EXIT_CRITERIA = N/A

…#155) Co-authored-by: Bertalan Kondrat <kb.pcre@gmail.com>

It was made a forced type in jOOQ because it will be needed in `InMemoryControlPlane` as well and it's not great to use DB types there.

auto create share group when alter offset

…ate" This reverts commit 05f8874, reversing changes made to 143e6cc.

…e#228) * Revert "Merge pull request apache#161 from showuon/cl3_ut" This reverts commit 55efc8d, reversing changes made to 1c8b6d3. * Revert "Merge pull request apache#155 from showuon/cl3_share_auto_create" This reverts commit 05f8874, reversing changes made to 143e6cc. * MINOR: Cleanups in Tools Module (3/n) (apache#20332) This PR aims at cleaning up the tools module further by getting rid of some extra code which can be replaced by `record` Reviewers: Chia-Ping Tsai <chia7712@gmail.com> * MINOR: kafka-stream-groups.sh should fail quickly if the partition leader is unavailable (apache#20271) This PR applies the same partition leader check for `StreamsGroupCommand` as `ShareGroupCommand` and `ConsumerGroupCommand` to avoid the command execution timeout. Reviewers: Lucas Brutschy <lucasbru@apache.org> * KAFKA-19662: Allow resetting offset for unsubscribed topic in kafka-share-groups.sh (apache#20453) The `kafka-share-groups.sh` tool checks whether a topic already has a start-offset in the share group when resetting offsets. This is not necessary. By removing the check, it is possible to set a start offset for a topic which has not yet but will be subscribed in the future, thus initialising the consumption point. There is still a small piece of outstanding work to do with resetting the offset for a non-existent group which should also create the group. A subsequent PR will be used to address that. Reviewers: Jimmy Wang <48462172+JimmyWang6@users.noreply.github.com>, Lan Ding <isDing_L@163.com>, Apoorv Mittal <apoorvmittal10@gmail.com> * KAFKA-19662: Reset share group offsets for unsubscribed topics (apache#20708) This PR allows the kafka-share-groups.sh --reset-offsets tool to be used to set offsets for topics which are not currently subscribed in a share group. It also works if the share group does not yet exist. This brings the capability in line with the equivalent function in Kafka-consumer-groups.sh. The primary purpose is to allow offsets to be set before the share group is first used as a way of initialising in a known state. Reviewers: Jimmy Wang <48462172+JimmyWang6@users.noreply.github.com>, Kuan-Po Tseng <brandboat@gmail.com>, Apoorv Mittal <apoorvmittal10@gmail.com> --------- Co-authored-by: Sanskar Jhajharia <sjhajharia@confluent.io> Co-authored-by: jimmy <wangzhiwang611@gmail.com> Co-authored-by: Andrew Schofield <aschofield@confluent.io>

persisting session information in Requests

d97449d

harshach reviewed Aug 21, 2015
View reviewed changes

adding logging per Harsha's advice

e55c50b

avoiding hardcoded ANONYMOUS string everywhere, per Parth's comment

d4d4ed7

ijuma reviewed Aug 21, 2015
View reviewed changes

eribeiro reviewed Aug 21, 2015
View reviewed changes

addressing suggestions by Ismael and Edward

ba8a334

junrao reviewed Aug 23, 2015
View reviewed changes

added security protocol and principal to request logs

8d40258

junrao reviewed Aug 26, 2015
View reviewed changes

minor fix for logs following Jun's comments

48821bf

asfgit closed this in 8b538d6 Aug 26, 2015

apurvam added a commit to apurvam/kafka that referenced this pull request Mar 31, 2017

First system test for the idempotent producer (apache#155)

1662bc2

Modified the replication test to check for duplicates if idempotence is enabled.

k0b3rIT added a commit to k0b3rIT/kafka that referenced this pull request Mar 24, 2025

CDPD-77731: Add health endpoint to ConnectAuthorizationFilter (apache…

e499950

…#155) Co-authored-by: Bertalan Kondrat <kb.pcre@gmail.com>

davide-armand pushed a commit to aiven/kafka that referenced this pull request Dec 1, 2025

Add new merge file reason [INK-101] (apache#155)

80b7fef

It was made a forced type in jOOQ because it will be needed in `InMemoryControlPlane` as well and it's not great to use DB types there.

fvaleri pushed a commit to fvaleri/kafka that referenced this pull request May 8, 2026

Merge pull request apache#155 from showuon/cl3_share_auto_create

05f8874

auto create share group when alter offset

gaurav-narula added a commit to gaurav-narula/kafka that referenced this pull request Jul 20, 2026

Revert "Merge pull request apache#155 from showuon/cl3_share_auto_cre…

6eea273

…ate" This reverts commit 05f8874, reversing changes made to 143e6cc.

Uh oh!

Conversation

gwenshap commented Aug 21, 2015

Uh oh!

asfbot commented Aug 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harshach commented Aug 21, 2015

Uh oh!

asfbot commented Aug 21, 2015

Uh oh!

Parth-Brahmbhatt commented Aug 21, 2015

Uh oh!

asfbot commented Aug 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Aug 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwenshap commented Aug 21, 2015

Uh oh!

ijuma commented Aug 21, 2015

Uh oh!

asfbot commented Aug 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Aug 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junrao commented Aug 26, 2015

Uh oh!

gwenshap commented Aug 26, 2015

Uh oh!

Parth-Brahmbhatt commented Aug 26, 2015

Uh oh!

gwenshap commented Aug 26, 2015

Uh oh!

asfbot commented Aug 26, 2015

Uh oh!

ijuma commented Aug 26, 2015

Uh oh!

Parth-Brahmbhatt commented Aug 26, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development