Skip to content

MINOR: remove unneccessary public keyword from ProducerInterceptor/ConsumerInterceptor interface#10746

Closed
KahnCheny wants to merge 29 commits into
apache:trunkfrom
KahnCheny:trunk
Closed

MINOR: remove unneccessary public keyword from ProducerInterceptor/ConsumerInterceptor interface#10746
KahnCheny wants to merge 29 commits into
apache:trunkfrom
KahnCheny:trunk

Conversation

@KahnCheny

@KahnCheny KahnCheny commented May 23, 2021

Copy link
Copy Markdown
Contributor

remove unneccessary public keyword from ProducerInterceptor interface

“KahnCheny” and others added 4 commits May 23, 2021 17:04
MINOR: remove unneccessary public keyword from ProducerInterceptor in…
d9bab79 
…terface
@showuon
MINOR: update java doc for deprecated methods (apache#10722)
efb7cda 
Reviewers: Matthias J. Sax <matthias@confluent.io>
@kowshik
MINOR: Add log identifier/prefix printing in Log layer static functio…
c92e62a 
…ns (apache#10742)

When apache#10478 was merged, we accidentally lost the identifier/prefix string that we used to previously log to stderr from some of the functions in the Log class. In this PR, I have reinstated the identifier/prefix logging in these functions, so that the debuggability is restored.

Reviewers: Luke Chen <showuon@gmail.com>, Cong Ding <cong@ccding.com>, Jun Rao <junrao@gmail.com>
@ableegoldman
MINOR: clarify message ordering with max in-flight requests and idemp…
33df5c6 
…otent producer (apache#10690)

The docs for the max.in.flight.requests.per.connection and enable.idempotence configs currently imply that setting the max in-flight request greater than 1 will break the message ordering guarantee, but that is only true if enable.idempotence is false. When using an idempotent producer, the max in-flight request can be up to 5 without re-ordering messages.

Reviewers: Matthias J. Sax <mjsax@confluent.io>, Ismael Juma <mlists@juma.me.uk>, Luke Chen <showuon@gmail.com>
showuon
showuon reviewed May 25, 2021
View reviewed changes

@showuon showuon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@KahnCheny , thanks for the PR. LGTM! Could you also remove the public keyword in ConsumerInterceptor ? Thank you.

@KahnCheny

KahnCheny commented May 25, 2021

Copy link
Copy Markdown
Contributor Author

@KahnCheny , thanks for the PR. LGTM! Could you also remove the public keyword in ConsumerInterceptor ? Thank you.

Of course.

@KahnCheny KahnCheny changed the title MINOR: remove unneccessary public keyword from ProducerInterceptor interface MINOR: remove unneccessary public keyword from ProducerInterceptor/ConsumerInterceptor interface May 25, 2021
showuon
showuon approved these changes May 25, 2021
View reviewed changes

@showuon showuon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! @KahnCheny , thanks for the PR!

@KahnCheny

KahnCheny commented May 25, 2021

Copy link
Copy Markdown
Contributor Author

Thanks

cursedquail and others added 4 commits May 25, 2021 15:12
@cursedquail
KAFKA-12800: Configure generator to fail on trailing JSON tokens (apa…
63b6db3 
…che#10717)

Reviewers: David Jacot <djacot@confluent.io>
@mjsax
MINOR: add window verification to sliding-window co-group test (apach…
77573d8 
…e#10745)

Reviewers: Luke Chen <showuon@gmail.com>, A. Sophie Blee-Goldman <sophie@confluent.io>
KAFKA-12260: Avoid hitting NPE for partitionsFor (apache#10017)
e4f2f6f 
Remove null pointer from the public partitionsFor API.

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
@dajac
MINOR: Log more information when producer snapshot is written (apache…
38e8391 
…#10757)

This patch logs more information when a producer snapshot is written to the disk.

Reviewers: Ismael Juma <mlists@juma.me.uk>, Lucas Bradstreet <lucas@confluent.io>
@KahnCheny

KahnCheny commented May 26, 2021

Copy link
Copy Markdown
Contributor Author

jenkins pr-merge passes?

ableegoldman and others added 15 commits May 26, 2021 10:35
@ableegoldman
MINOR: deprecate TaskMetadata constructor and add KIP-740 notes to up…
bbe170a 
…grade guide (apache#10755)

Quick followup to KIP-740 to actually deprecate this constructor, and update the upgrade guide with what we changed in KIP-740. I also noticed the TaskId#parse method had been modified previously, and should be re-added to the public TaskId class. It had no tests, so now it does

Reviewers: Matthias J. Sax <mjsax@confluent.io>, Luke Chen <showuon@gmail.com>
@kowshik
MINOR: Log constructor: Flip logical NOT for readability (apache#10756)
47094ef 
Reviewers: Jun Rao <junrao@gmail.com>
@prince-mahajan
Update implementation.html (apache#10771)
db288e4 
Fixing the link to a cited blog. The existing link now points to a steroid website so need to pull the blog from internet archives.
@mdedetrich
KAFKA-12819: Add assert messages to MirrorMaker tests plus other qual…
56d9482 
…ity of life improvements (apache#10762)

Reviewers: Ryanne Dolan <ryannedolan@gmail.com>, Konstantine Karantasis <k.karantasis@gmail.com>
@jlprat
KAFKA-12796: Removal of deprecated classes under streams-scala (apach…
a02b19c 
…e#10710)

Removes previously deprecated methods in older KIPs

Reviewers: Bruno Cadonna <cadonna@apache.org>
@ijuma
MINOR: Adjust parameter ordering of waitForCondition and `retryOnEx…
e4b3a3c 
…ceptionWithTimeout` (apache#10759)

New parameters in overloaded methods should appear later apart from
lambdas that should always be last.
@kowshik
MINOR: Improve Log layer segment iteration logic and few other areas (a…
3fb836f 
…pache#10684)

In Log.collectAbortedTransactions() I've restored a previously used logic, such that it would handle the case where the starting segment could be null. This was the case previously, but the PR apache#10401 accidentally changed the behavior causing the code to assume that the starting segment won't be null.

In Log.rebuildProducerState() I've removed usage of the allSegments local variable. The logic looks a bit simpler after I removed it.

I've introduced a new LogSegments.higherSegments() API. This is now used to make the logic a bit more readable in Log. collectAbortedTransactions() and Log.deletableSegments() APIs.

I've removed the unnecessary use of java.lang.Long in LogSegments class' segments map definition.

I've converted a few LogSegments API from public to private, as they need not be public.

Reviewers: Ismael Juma <ismael@juma.me.uk>, Cong Ding <cong@ccding.com>, Jun Rao <junrao@gmail.com>
@ijuma
KAFKA-12856: Upgrade Jackson to 2.12.3 (apache#10778)
2313026 
2.10.x is no longer supported, so we should move to 2.12 for the 3.0
release.

ScalaObjectMapper has been deprecated and it looks like we don't
actually need it, so remove its usage.

Reviewers: David Jacot <djacot@confluent.io>
@ijuma
MINOR: Don't ignore deletion of partition metadata file and log topic…
6659777 
… id clean-ups (apache#10761)

Log if deletion fails and don't expose log topic id for mutability outside of `assignTopicId()`.

Also remove an unnecessary parameter in `PartitionTest`.

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Justine Olshan <jolshan@confluent.io>
@lbradstreet
KAFKA-12791: ConcurrentModificationException in AbstractConfig use by…
5cbe6a7 
… KafkaProducer (apache#10704)

Recently we have noticed multiple instances where KafkaProducers have failed to constructor due to the following exception:

```
org.apache.kafka.common.KafkaException: Failed to construct kafka producer at 
org.apache.kafka.clients.producer.KafkaProducer.<init>(KafkaProducer.java:440) at 
org.apache.kafka.clients.producer.KafkaProducer.<init>(KafkaProducer.java:291) at 
org.apache.kafka.clients.producer.KafkaProducer.<init>(KafkaProducer.java:318) 
java.base/java.lang.Thread.run(Thread.java:832) Caused by: java.util.ConcurrentModificationException at 
java.base/java.util.HashMap$HashIterator.nextNode(HashMap.java:1584) at 
java.base/java.util.HashMap$KeyIterator.next(HashMap.java:1607) at 
java.base/java.util.AbstractSet.removeAll(AbstractSet.java:171) at 
org.apache.kafka.common.config.AbstractConfig.unused(AbstractConfig.java:221) at 
org.apache.kafka.common.config.AbstractConfig.logUnused(AbstractConfig.java:379) at 
org.apache.kafka.clients.producer.KafkaProducer.<init>(KafkaProducer.java:433) ... 9 more 
exception.class:org.apache.kafka.common.KafkaException exception.message:Failed to construct kafka producer
```

This is due to the fact that `used` below is a synchronized set. `used` is being modified while removeAll is being called. This is due to the use of RecordingMap in the Sender thread (see below). Switching to a ConcurrentHashSet avoids this issue as it support concurrent iteration.

```
	at org.apache.kafka.clients.producer.ProducerConfig.ignore(ProducerConfig.java:569)
	at org.apache.kafka.common.config.AbstractConfig$RecordingMap.get(AbstractConfig.java:638)
	at org.apache.kafka.common.network.ChannelBuilders.createPrincipalBuilder(ChannelBuilders.java:242)
	at org.apache.kafka.common.network.PlaintextChannelBuilder$PlaintextAuthenticator.<init>(PlaintextChannelBuilder.java:96)
	at org.apache.kafka.common.network.PlaintextChannelBuilder$PlaintextAuthenticator.<init>(PlaintextChannelBuilder.java:89)
	at org.apache.kafka.common.network.PlaintextChannelBuilder.lambda$buildChannel$0(PlaintextChannelBuilder.java:66)
	at org.apache.kafka.common.network.KafkaChannel.<init>(KafkaChannel.java:174)
	at org.apache.kafka.common.network.KafkaChannel.<init>(KafkaChannel.java:164)
	at org.apache.kafka.common.network.PlaintextChannelBuilder.buildChannel(PlaintextChannelBuilder.java:79)
	at org.apache.kafka.common.network.PlaintextChannelBuilder.buildChannel(PlaintextChannelBuilder.java:67)
	at org.apache.kafka.common.network.Selector.buildAndAttachKafkaChannel(Selector.java:356)
	at org.apache.kafka.common.network.Selector.registerChannel(Selector.java:347)
	at org.apache.kafka.common.network.Selector.connect(Selector.java:274)
	at org.apache.kafka.clients.NetworkClient.initiateConnect(NetworkClient.java:1097)
	at org.apache.kafka.clients.NetworkClient.access$700(NetworkClient.java:87)
	at org.apache.kafka.clients.NetworkClient$DefaultMetadataUpdater.maybeUpdate(NetworkClient.java:1276)
	at org.apache.kafka.clients.NetworkClient$DefaultMetadataUpdater.maybeUpdate(NetworkClient.java:1164)
	at org.apache.kafka.clients.NetworkClient.poll(NetworkClient.java:637)
	at org.apache.kafka.clients.producer.internals.Sender.runOnce(Sender.java:327)
	at org.apache.kafka.clients.producer.internals.Sender.run(Sender.java:242)
```

Reviewers: Ismael Juma <ismael@juma.me.uk>
@chia7712
MINOR: make sure all fiedls of o.p.k.s.a.Action are NOT null (apache#…
a6f3094 
…10764)

I'm migrating Ranger's kafka plugin from deprecated Authorizer (this is already removed by 976e78e) to new API (see https://issues.apache.org/jira/browse/RANGER-3231). The kafka plugin needs to take something from field resourcePattern but it does not know whether the field is nullable (or users need to add null check). I check all usages and I don't observe any null case.

Reviewers: Ismael Juma <ismael@juma.me.uk>
@jlprat
KAFKA-12782: Fix Javadocs generation by upgrading JDK (apache#10780)
588ea44 
This, upgrades JDK to version 15 for the docs generation, this way we
can circumvent bug https://bugs.openjdk.java.net/browse/JDK-8215291
present in JDK11

Reviewers: Ismael Juma <ismael@juma.me.uk>
@vvcephei
KAFKA-8410: KTableProcessor migration groundwork (apache#10744)
f207bac 
* Lay the groundwork for migrating KTable Processors to the new PAPI.
* Migrate the KTableFilter processor to prove that the groundwork works.

This is an effort to help break up apache#10507 into multiple PRs.

Reviewers: Boyang Chen <boyang@apache.org>
@chia7712
MINOR: upgrade pip from 20.2.2 to 21.1.1 (apache#10661)
0bf4b47 
The following error happens on my mac m1 when building docker image for system tests.

Collecting pynacl
  Using cached PyNaCl-1.4.0.tar.gz (3.4 MB)
  Installing build dependencies ... error
  ERROR: Command errored out with exit status 1:
   command: /usr/bin/python3 /usr/local/lib/python3.8/dist-packages/pip install --ignore-installed --no-user --prefix /tmp/pip-build-env-k867aac0/overlay --no-warn-script-location --no-binary :none: --only-binary :none: -i https://pypi.org/simple -- 'setuptools>=40.8.0' wheel 'cffi>=1.4.1; python_implementation != '"'"'PyPy'"'"''
       cwd: None
  Complete output (14 lines):
  Traceback (most recent call last):
    File "/usr/lib/python3.8/runpy.py", line 194, in _run_module_as_main
      return _run_code(code, main_globals, None,
    File "/usr/lib/python3.8/runpy.py", line 87, in _run_code
      exec(code, run_globals)
    File "/usr/local/lib/python3.8/dist-packages/pip/__main__.py", line 23, in <module>
      from pip._internal.cli.main import main as _main  # isort:skip # noqa
    File "/usr/local/lib/python3.8/dist-packages/pip/_internal/cli/main.py", line 5, in <module>
      import locale
    File "/usr/lib/python3.8/locale.py", line 16, in <module>
      import re
    File "/usr/lib/python3.8/re.py", line 145, in <module>
      class RegexFlag(enum.IntFlag):
  AttributeError: module 'enum' has no attribute 'IntFlag'
  ----------------------------------------
ERROR: Command errored out with exit status 1: /usr/bin/python3 /usr/local/lib/python3.8/dist-packages/pip install --ignore-installed --no-user --prefix /tmp/pip-build-env-k867aac0/overlay --no-warn-script-location --no-binary :none: --only-binary :none: -i https://pypi.org/simple -- 'setuptools>=40.8.0' wheel 'cffi>=1.4.1; python_implementation != '"'"'PyPy'"'"'' Check the logs for full command output.

There was a related issue: pypa/pip#9689 and it is already fixed by pypa/pip#9689 (included by pip 21.1.1). I test the pip 21.1.1 and it works well on mac m1.

Reviewers: Ismael Juma <ismael@juma.me.uk>
@rohit-sachan
KAFKA-12865 : Fix doc error in Admin.describeAcls API (apache#10790)
99a0312 
Reviewers: Lee Dongjin <dongjin@apache.org>, Manikumar Reddy <manikumar.reddy@gmail.com>
ijuma and others added 5 commits May 30, 2021 12:16
@ijuma
MINOR: Reduce allocations in requests via buffer caching (apache#9229)
6b005b2 
Use a caching `BufferSupplier` per request handler thread so that
decompression buffers are cached if supported by the underlying
`CompressionType`. This achieves a similar outcome as apache#9220, but
with less contention.

We introduce a `RequestLocal` class to make it easier to introduce
new request scoped stateful instances (one example we discussed
previously was an `ActionQueue` that could be used to avoid
some of the complex group coordinator locking).

This is a small win for zstd (no synchronization or soft references) and
a more significant win for lz4. In particular, it reduces allocations
significantly when the number of partitions is high. The decompression
buffer size is typically 64 KB, so a produce request with 1000 partitions
results in 64 MB of allocations even if each produce batch is small (likely,
when there are so many partitions).

I did a quick producer perf local test with 5000 partitions, 1 KB record
size,
1 broker, lz4 and ~0.5 for the producer compression rate metric:

Before this change:
> 20000000 records sent, 346314.349535 records/sec (330.27 MB/sec),
148.33 ms avg latency, 2267.00 ms max latency, 115 ms 50th, 383 ms 95th, 777 ms 99th, 1514 ms 99.9th.

After this change:
> 20000000 records sent, 431956.113259 records/sec (411.95 MB/sec),
117.79 ms avg latency, 1219.00 ms max latency, 99 ms 50th, 295 ms 95th, 440 ms 99th, 662 ms 99.9th.

That's a 25% throughput improvement and p999 latency was reduced to
under half (in this test).

Default arguments will be removed in a subsequent PR.

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
@chia7712
HOTFIX: fix build error (apache#10796)
6c9e27c 
Fix compile error in scala tests.

The compile error is:

```
[Error] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-9229/core/src/test/scala/unit/kafka/server/metadata/BrokerMetadataListenerTest.scala:97: polymorphic expression cannot be instantiated to expected type;

[2021-05-29T02:34:50.308Z]  found   : [T]()T

[2021-05-29T02:34:50.308Z]  required: kafka.server.RequestLocal
```

This error happens only in scala 2.12

Reviewers: Bruno Cadonna <cadonna@apache.org>
MINOR: remove unneccessary public keyword from ProducerInterceptor in…
ee88bf6 
…terface
MINOR: remove unneccessary public keyword from ConsumerInterceptor in…
dcc83c2 
…terface
Merge remote-tracking branch 'origin/trunk' into trunk
5e6f627
@showuon

showuon commented Jun 1, 2021

Copy link
Copy Markdown
Member

@KahnCheny , it looks like you made some change after I approved this PR. Now, there are 162 files changed. Could you check again? Thanks.

@KahnCheny KahnCheny closed this Jun 1, 2021
@KahnCheny
KahnCheny deleted the branch apache:trunk June 1, 2021 12:17
@KahnCheny
KahnCheny deleted the trunk branch June 1, 2021 12:17
@KahnCheny

KahnCheny commented Jun 1, 2021 via email

Copy link
Copy Markdown
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@showuon showuon showuon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.