Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-2145: Add a log config so users can define topic owners. #77

Closed
wants to merge 5 commits into from
Closed

KAFKA-2145: Add a log config so users can define topic owners. #77

wants to merge 5 commits into from

Conversation

Parth-Brahmbhatt
Copy link
Contributor

No description provided.

@Parth-Brahmbhatt
Copy link
Contributor Author

@junrao Please review and I would recommend not to merge this until KAFKA-2210 is merged in at which point we should be able to leverage KafkaPrincipal instance as owner field.

@Parth-Brahmbhatt
Copy link
Contributor Author

@ijuma I am not sure why our comments disappeared. I have removed the return statements and test still passes.

@ijuma
Copy link
Contributor

ijuma commented Jul 15, 2015

@Parth-Brahmbhatt If a commit changes and the branch is force pushed, the comments can disappear. Thanks for making the change.

@asfbot
Copy link

asfbot commented Aug 25, 2015

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

junrao
junrao reviewed Aug 31, 2015
View reviewed changes
core/src/main/scala/kafka/admin/AdminUtils.scala Outdated
//by default we make user that issues topic creation as the owner.
if (!config.containsKey(LogConfig.OwnersProp)) {
config.put(LogConfig.OwnersProp, System.getProperty("user.name"))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we should get this from the user.name system property. Would it be better to just use the default owner if it's not specified explicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I preferred this way as we can establish some sort of true identity rather than complete anonymity. Once we have sasl support we can actually derive identities based on who the logged in sasl user is instead of asking the user to pass this as an argument or reliying on System.getProperty("user.name"). can you elaborate why it is better to just leave it as anonymous.

@asfbot
Copy link

asfbot commented Sep 16, 2015

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

@Parth-Brahmbhatt
Copy link
Contributor Author

@junrao I have made changes so this PR now users KafkaPrincipal. One open issue is how to handle multiple owners. I originally tested by using , as separator in the test. However I realized this won't work from CLI as CLI already uses , to distinguish different keys e.g.

bin/kafka-topics.sh --create --zookeeper localhost:2181 --topic owner-test-config-3 --partitions 1 --config owners=User:alice,flush.ms=10000 will work as we are only specifying one owner.

However bin/kafka-topics.sh --create --zookeeper localhost:2181 --topic owner-test-config-3 --partitions 1 --config owners=User:alice,User:Bob,flush.ms=10000 will not work as CLI will try to match User:Bob as Key=Val and fail with "all configs to be added must be in the format "key=val"" Exception.

We could either use some different delimiter or just add --owner as a new option. Please let me know which option makes more sense to you or if you have an alternative approach.

@Parth-Brahmbhatt
Copy link
Contributor Author

ASFBOT reported a transient unit test failure

kafka.consumer.MetricsTest > testMetricsLeak FAILED
java.lang.AssertionError: expected:<177> but was:<179>
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:743)
at org.junit.Assert.assertEquals(Assert.java:118)
at org.junit.Assert.assertEquals(Assert.java:555)
at org.junit.Assert.assertEquals(Assert.java:542)
at kafka.consumer.MetricsTest$$anonfun$testMetricsLeak$1.apply$mcVI$sp(MetricsTest.scala:65)
at scala.collection.immutable.Range.foreach$mVc$sp(Range.scala:141)
at kafka.consumer.MetricsTest.testMetricsLeak(MetricsTest.scala:63)

unrelated to this change

@guozhangwang
Copy link
Contributor

@junrao Could you take another look at this PR and see if it is still valid? If yes @Parth-Brahmbhatt could rebase the PR and continue addressing your comments.

@asfbot
Copy link

asfbot commented Dec 21, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/316/
Test FAILed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented Dec 21, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/315/
Test FAILed (JDK 7 and Scala 2.10).

@asfbot
Copy link

asfbot commented Dec 21, 2016

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/317/
Test FAILed (JDK 8 and Scala 2.11).

@junrao
Copy link
Contributor

junrao commented Dec 21, 2016

  1. For specifying multiple owners, we can extend kafka-topics to do similar things as ConfigCommand that supports k2=[v1,v2,v2].

  2. Now that we have a createTopicRequest, the owner probably needs to be set on the broker side.

  3. It would be useful to think through other use cases of owner. For example, maybe the owner can delete a topic by default?

allenxwang pushed a commit to allenxwang/kafka that referenced this pull request Aug 24, 2018
Merge pull request apache#77 in DATA_PIPELINE/kafka from ~AWANG/kafka…
8d3ddf2 
…:1.1.1-sync to 1.1-nflx

* commit '9611672e287c1a7933a78590e3f381da2ae7d136': (57 commits)
  MINOR: increase dev version from 1.1.1-SNAPSHOT to 1.1.2-SNAPSHOT (apache#5409)
  MINOR: Add thread dumps if broker node cannot be stopped (apache#5373)
  MINOR: update release.py
  MINOR: fix upgrade docs for Streams (apache#5392)
  MINOR: improve docs version numbers (apache#5372)
  Update version on the branch to 1.1.2-SNAPSHOT
  KAFKA-6292; Improve FileLogInputStream batch position checks to avoid type overflow (apache#4928)
  HOTFIX: Fix checkstyle errors in MetricsTest (apache#5345)
  KAFKA-7136: Avoid deadlocks in synchronized metrics reporters (apache#5341)
  MINOR: Close timing window in SimpleAclAuthorizer startup (apache#5318)
  MINOR: Use kill_java_processes when killing ConsoleConsumer in system tests (apache#5297)
  KAFKA-7104: More consistent leader's state in fetch response (apache#5305)
  Revert "MINOR: Avoid coarse lock in Pool#getAndMaybePut (apache#5258)"
  MINOR: Avoid coarse lock in Pool#getAndMaybePut (apache#5258)
  MINOR: bugfix streams total metrics (apache#5277)
  KAFKA-7082: Concurrent create topics may throw NodeExistsException (apache#5259)
  MINOR: Upgrade to Gradle 4.8.1
  KAFKA-7012: Don't process SSL channels without data to process (apache#5237)
  KAFKA-7058: Comparing schema default values using Objects#deepEquals()
  KAFKA-7047: Added SimpleHeaderConverter to plugin isolation whitelist
  ...
efeg pushed a commit to efeg/kafka that referenced this pull request Jan 29, 2020
@alexandrfox @becketqin
Disabled debug mode by default (apache#77)
ec73f46
radai-rosenblatt added a commit to radai-rosenblatt/kafka that referenced this pull request Apr 15, 2020
@radai-rosenblatt
[LI-HOTFIX] KAFKA-9855 - return cached Structs for Schemas with no fi…
c70c676 
…elds (apache#77)

TICKET =
LI_DESCERIPTION = committing to our fork until upstream picks this up
EXIT_CRITERIA = when KAFKA-9855 is accepted upstream (specifically - apache#8472)

Signed-off-by: radai-rosenblatt <radai.rosenblatt@gmail.com>
smccauliff pushed a commit to smccauliff/kafka that referenced this pull request Oct 8, 2020
@radai-rosenblatt @gitlw
[LI-HOTFIX] KAFKA-9855 - return cached Structs for Schemas with no fi…
a1bd941 
…elds (apache#77)

TICKET =
LI_DESCERIPTION = committing to our fork until upstream picks this up
EXIT_CRITERIA = when KAFKA-9855 is accepted upstream (specifically - apache#8472)

Signed-off-by: radai-rosenblatt <radai.rosenblatt@gmail.com>
rustd pushed a commit to rustd/pranavfinaldemokafka that referenced this pull request Feb 9, 2024
@rutvijmehta-harness @hachikuji
MINOR: Fix PartitionRegistration.hashCode (apache#12774) (apache#77)
ec53238 
`PartitionRegistration.hashCode` passes raw arrays to `Objects.hash` in the `hashCode` implementation. This doesn't work since the `equals` implementation uses `Arrays.equals`. We should use `Arrays.hashCode` instead. 

Reviewers: David Arthur <mumrah@gmail.com>

Co-authored-by: Jason Gustafson <jason@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@Parth-Brahmbhatt @ijuma @asfbot @guozhangwang @junrao