Update#1
Merged
Merged
Conversation
…ent overrides (#6789) Because of how config values are converted into strings in the `AbstractHerder.validateClientOverrides()` method after being validated by the client override policy, an exception is thrown if the value returned by the policy isn't already parsed as the type expected by the client `ConfigDef`. The fix here involves parsing client override properties before passing them to the override policy. A unit test is added to ensure that several different types of configs are validated properly by the herder. Author: Chris Egerton <chrise@confluent.io> Reviewers: Magesh Nandakumar <magesh.n.kumar@gmail.com>, Randall Hauch <rhauch@gmail.com>
This patch attempts to simplify the interaction between Partition and the various components from `ReplicaManager`. This is primarily to make unit testing easier. I have also tried to eliminate the OfflinePartition sentinel which has always been unsafe. Reviewers: Boyang Chen <bchen11@outlook.com>, David Arthur <mumrah@gmail.com>
These are important to ensure we don't break compatibility. Author: Ismael Juma <ismael@juma.me.uk> Reviewers: Gwen Shapira Closes #6794 from ijuma/update-version-compat-tests
Important fix: FasterXML/jackson-databind#2326 Reviewers: Colin P. McCabe <cmccabe@apache.org>
Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>
Reviewers: Boyang Chen <bchen11@outlook.com>, Jason Gustafson <jason@confluent.io>
Reviewers: John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>, Guozhang Wang <guozhang@confluent.io>, Boyang Chen <boyang@confluent.io>
The old docs here used a now deprecated method to set the block cache size. In switching over to the new one we would now need to construct a Cache object and therefore also need to close it, so this is a good opportunity to demonstrate the RocksDBConfigSetter#close method that will need to be implemented by users. Reviewers: Guozhang Wang <wangguoz@gmail.com>
…6723) An API call for consumer groups must send a FindCoordinatorRequest to find the consumer group coordinator, and then send a follow-up request to that node. But the coordinator might move after the FindCoordinatorRequest but before the follow-up request is sent. In that case we currently fail. This change fixes that by detecting this error and then retrying. This fixes listConsumerGroupOffsets, deleteConsumerGroups, and describeConsumerGroups. Reviewers: Colin P. McCabe <cmccabe@apache.org>, Boyang Chen <bchen11@outlook.com>
This gets hit when debug logging is enabled. Reviewers: Bill Bejeck <bill@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
…pports latest version (#6806) In the olden days, OffsetForLeaderEpoch was exclusively an inter-broker protocol and required Cluster level permission. With KIP-320, clients can use this API as well and so we lowered the required permission to Topic Describe. The only way the client can be sure that the new permissions are in use is to require version 3 of the protocol which was bumped for 2.3. If the broker does not support this version, we skip the validation and revert to the old behavior. Additionally, this patch fixes a problem with the newly added replicaId field when parsed from older versions which did not have it. If the field was not present, then we used the consumer's sentinel value, but this would limit the range of visible offsets by the high watermark. To get around this problem, this patch adds a separate "debug" sentinel similar to APIs like Fetch and ListOffsets. Reviewers: Ismael Juma <ismael@juma.me.uk>
…ents (#6722) When cleaning transactional data, we need to keep track of which transactions still have data associated with them so that we do not remove the markers. We had logic to do this, but the state was not being carried over when beginning cleaning for a new set of segments. This could cause the cleaner to incorrectly believe a transaction marker was no longer needed. The fix here carries the transactional state between groups of segments to be cleaned. Reviewers: Dhruvil Shah <dhruvil@confluent.io>, Viktor Somogyi <viktorsomogyi@gmail.com>, Jason Gustafson <jason@confluent.io>
…ion is possible (#6823) The consumer should await api version information before determining whether the broker supports offset validation. In KAFKA-8422, we skip the validation if we don't have api version information, which means we always skip validation the first time we connect to a node. This bug was detected by the failing system test `tests/client/truncation_test.py`. The test passes again with this fix. Reviewers: Ismael Juma <ismael@juma.me.uk>
* add doc changes for static membership release * address comments * address comments for a separate page * address Matthias' comment
…6808) Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <mjsax@apache.org>, Bill Bejeck <bbejeck@gmail.com>
…ker (#6812) Authorized operations must be null when talking to a pre-KIP-430 broker. If we present this as the empty set instead, it is impossible for clients to know if they have no permissions, or are talking to an old broker. Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>
Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, Guozhang Wang <guozhang@confluent.io>, Boyang Chen <boyang@confluent.io>
…ager (#6813) We have two fields `highWatermarkCheckPointThreadStarted` and `hwThreadInitialized` which appear to be serving the same purpose. This patch gets rid of `hwThreadInitialized`. Reviewers: Guozhang Wang <wangguoz@gmail.com>
…6831) As we are planning to add on more supporting features for rebalancing under static membership, we need to make sure the behavior for `group.instance.id` is consistent throughout the whole stack. This patch ensures that the default value is null in the JoinGroup response. Reviewers: Guozhang Wang <wangguoz@gmail.com>, Jason Gustafson <jason@confluent.io>
…nstance.id` are valid combo (#6830) As title suggests, this unit test is just a double check. No need to push in 2.3 Reviewers: Guozhang Wang <wangguoz@gmail.com>, Matthias J. Sax <mjsax@apache.org>
…ges (#6816) I think it's better just to make single-batch as a universal requirement for all versions for compressed messages, and for V2 and beyond uncompressed messages as well. Reviewers: Jason Gustafson <jason@confluent.io>
…before checking for the required --partitions flag (#6786) Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>
…value store (#6825) Guozhang Wang <wangguoz@gmail.com>, Bill Bejeck <bbejeck@gmail.com>
…#6834) The test was failing because other test cases are leaving some metrics in the registry. This patch cleans the registry before and after test runs. Reviewers: Ismael Juma <ismael@juma.me.uk>
Implements KIP-460: https://cwiki.apache.org/confluence/display/KAFKA/KIP-460%3A+Admin+Leader+Election+RPC. Reviewers: Jun Rao <junrao@gmail.com>, Jason Gustafson <jason@confluent.io>
Reviewers: Guozhang Wang <wangguoz@gmail.com>, Bruno Cadonna <bruno@confluent.io>
…#6838) Author: Alex Diachenko <sansanichfb@gmail.com> Reviewer: Randall Hauch <rhauch@gmail.com>
When poll is called which resets the offsets to the beginning, followed by a seekToEnd and a position, it could happen that the "reset to earliest" call in poll overrides the "reset to latest" initiated by seekToEnd in a very delicate way: 1. both request has been issued and returned to the client side (listOffsetResponse has happened) 2. in Fetcher.resetOffsetIfNeeded(TopicPartition, Long, OffsetData) the thread scheduler could prefer the heartbeat thread with the "reset to earliest" call, overriding the offset to the earliest and setting the SubscriptionState with that position. 3. The thread scheduler continues execution of the thread (application thread) with the "reset to latest" call and discards it as the "reset to earliest" already set the position - the wrong one. 4. The blocking position call returns with the earliest offset instead of the latest, despite it wasn't expected. The fix makes SubscriptionState synchronized so that we can verify that the reset is expected while holding the lock. Reviewers: Jason Gustafson <jason@confluent.io>
A lot of confusion seems to have arisen from the StreamBuilder#addGlobalStore(...ProcessorSupplier) method. Users have assumed they can safely use this to transform records before populating their global state store; unfortunately this results in corrupted data as on restore the records are read directly from the source topic changelog, bypassing their custom processor. We should probably provide a means to do this at some point but for the time being we should clarify the proper use of #addGlobalStore as it currently functions Reviewers: Matthias J. Sax <mjsax@apache.org>, Bruno Cadonna <bruno@confluent.io>
#6997) Implement KIP-480, which specifies that the default partitioner should use a "sticky" partitioning strategy for records that have a null key. Reviewers: Colin P. McCabe <cmccabe@apache.org>, Lucas Bradstreet <lucasbradstreet@gmail.com>, Stanislav Kozlovski <stanislav_kozlovski@outlook.com>, Jun Rao <junrao@gmail.com>, Kamal Chandraprakash <kamal.chandraprakash@gmail.com>
…partitions (#6988) This is an updated implementation of #5844 by @MayureshGharat (with Mayuresh's permission). As described in the original ticket: > Today when we call KafkaConsumer.poll(), it will fetch data from Kafka asynchronously and is put in to a local buffer (completedFetches). > > If now we pause some TopicPartitions and call KafkaConsumer.poll(), we might throw away any buffered data that we might have in the local buffer for these TopicPartitions. Generally, if an application is calling pause on some TopicPartitions, it is likely to resume those TopicPartitions in near future, which would require KafkaConsumer to re-issue a fetch for the same data that it had buffered earlier for these TopicPartitions. This is a wasted effort from the application's point of view. This patch fixes the problem by retaining the paused data in the completed fetches queue, essentially moving it to the back on each call to `fetchedRecords`. Reviewers: Jason Gustafson <jason@confluent.io>
Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>
* Adds RocksDBMetrics class that provides methods to get sensors from the Kafka metrics registry and to setup the sensors to record RocksDB metrics * Extends StreamsMetricsImpl with functionality to add the required metrics to the sensors. Reviewers: Boyang Chen <boyang@confluent.io>, Bill Bejeck <bill@confluent.io>, Matthias J. Sax <matthias@confluent.io>, John Roesler <vvcephei@users.noreply.github.com>, Guozhang Wang <wangguoz@gmail.com>
<!-- Is there any breaking changes? If so this is a major release, make sure '#major' is in at least one commit message to get CI to bump the major. This will prevent automatic down stream dependency bumping / consuming. For more information about semantic versioning see: https://semver.org/ Suggested PR template: Fill/delete/add sections as needed. Optionally delete any commented block. --> What ---- <!-- Briefly describe **what** you have changed and **why**. Optionally include implementation strategy. --> References ---------- [**KIP-412**](https://cwiki.apache.org/confluence/display/KAFKA/KIP-412%3A+Extend+Admin+API+to+support+dynamic+application+log+levels) [**KAFKA-7800**](https://issues.apache.org/jira/browse/KAFKA-7800) [**Discussion Thread**](http://mail-archives.apache.org/mod_mbox/kafka-dev/201901.mbox/%3CCANZZNGyeVw8q%3Dx9uOQS-18wL3FEmnOwpBnpJ9x3iMLdXY3gEug%40mail.gmail.com%3E) [**Vote Thread**](http://mail-archives.apache.org/mod_mbox/kafka-dev/201902.mbox/%3CCANZZNGzpTJg5YX1Gpe5S%3DHSr%3DXGvmxvYLTdA3jWq_qwH-UvorQ%40mail.gmail.com%3E) <!-- Copy&paste links: to Jira ticket, other PRs, issues, Slack conversations... For code bumps: link to PR, tag or GitHub `/compare/master...master` --> Test&Review ------------ Test cases covered: * DescribeConfigs * Alter the log level with and without validateOnly, validate the results with DescribeConfigs Open questions / Follow ups -------------------------- If you're a reviewer, I'd appreciate your thoughts on these questions I have open: 1. Should we add synchronization to the Log4jController methods? - Seems like we don't get much value from it 2. Should we instantiate a new Log4jController instead of it having static methods? - All operations are stateless, so I thought static methods would do well 3. A logger which does not have a set value returns "null" (as seen in the unit tests). Should we just return the Root logger's level? Author: Stanislav Kozlovski <familyguyuser192@windowslive.com> Reviewers: Gwen Shapira Closes #6903 from stanislavkozlovski/KAFKA-7800-dynamic-log-levels-admin-ap
…Client (#7157) Changing the return type in an interface is a binary incompatible change. Reviewers: Jason Gustafson <jason@confluent.io>
Reviewer: Matthias J. Sax <matthias@confluent.io>
Reviews: A. Sophie Blee-Goldman <sophie@confluent.io>, Bill Bejeck <bill@confluent.io>, Bruno Cadonna <bruno@confluent.io>, Boyang Chen <boyang@confluent.io>, Guozhang Wang <guozhang@confluent.io>
… duration [JIRA](https://issues.apache.org/jira/browse/KAFKA-6263) - Add metrics to provide visibility for how long group metadata and transaction metadata take to load in order to understand some inactivity seen in the consumer groups - Tests include mocking load times by creating a delay after each are loaded and ensuring the measured JMX metric is as it should be Author: anatasiavela <anastasiavela@berkeley.edu> Reviewers: Gwen Shapira, Jason Gustafson Closes #7045 from anatasiavela/KAFKA-6263
… coverage (#7159) LogConfig.getConfigValue would throw a NoSuchElementException if any log config was defined without a server default mapping. Added a unit test for `getConfigValue` and a sanity test for `toHtml`/`toRst`/`toEnrichedRst`, which were previously not exercised during the test suite. Reviewers: Jason Gustafson <jason@confluent.io>, José Armando García Sancio <jsancio@users.noreply.github.com>
…pl` (#7161) Renames method names in StreamsMetricsImpl to make them consistent. Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, Guozhang Wang <wangguoz@gmail.com>
…n size() == 0 (#7164) Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <guozhang@confluent.io>
Author: Colin P. Mccabe <cmccabe@confluent.io> Reviewers: Gwen Shapira Closes #7058 from cmccabe/rr-style-fixes
2.9.9.1 and 2.9.9.2 include security fixes while 2.9.9.3 fixes a regression introduced in 2.9.9.2. Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>
Author: Mickael Maison <mickael.maison@gmail.com> Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>, Viktor Somogyi <viktorsomogyi@gmail.com> Closes #7098 from mimaison/KAFKA-8599
Scala 2.12.9 brings another 5% ~ 10% improvement in compiler performance, improved compatibility with JDK 11/12/13, and experimental infrastructure for build pipelining. zstd update includes performance improvements, among which the primary improvement is that decompression is ~7% faster. Level | v1.4.0 | v1.4.1 | Delta -- | -- | -- | -- 1 | 1390 MB/s | 1453 MB/s | +4.5% 3 | 1208 MB/s | 1301 MB/s | +7.6% 5 | 1129 MB/s | 1233 MB/s | +9.2% 7 | 1224 MB/s | 1347 MB/s | +10.0% 16 | 1278 MB/s | 1430 MB/s | +11.8% Jetty 9.4.19 includes a number of bug fixes: https://github.com/eclipse/jetty.project/releases/tag/jetty-9.4.19.v20190610 Mockito 3.0.0 switched the Java requirement from 7 to 8. Several updates to owaspDepCheckPlugin (4.0.2 -> 5.2.1). The rest are patch updates. Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>
…etadata/subscription change (#6884) 1. Add onPartitionsLost into the RebalanceListener, which will be triggered when the consumer found that the generation is reset due to fatal errors in response handling. 2. Semantical behavior change: with COOPERATIVE protocol, if the revoked / lost partitions are empty, do not trigger the corresponding callback at all. For added partitions though, even if it is empty we would still trigger the callback as a way to notify the rebalance event; with EAGER protocol, revoked / assigned callbacks are always triggered. The ordering of the callback would be the following: a. Callback onPartitionsRevoked / onPartitionsLost triggered. b. Update the assignment (both revoked and added). c. Callback onPartitionsAssigned triggered. In this way we are assured that users can still access the partitions being revoked, whereas they can also access the partitions being added. 3. Semantical behavior change (KAFKA-4600): if the rebalance listener throws an exception, pass it along all the way to the consumer.poll caller, but still completes the rest of the actions. Also, the newly assigned partitions list does not gets affected with exception thrown since it is just for notifying the users. 4. Semantical behavior change: the ConsumerCoordinator would not try to modify assignor's returned assignments, instead it will validate that assignments and set the error code accordingly: if there are overlaps between added / revoked partitions, it is a fatal error and would be communicated to all members to stop; if revoked is not empty, it is an error indicate re-join; otherwise, it is normal. 5. Minor: with the error code removed from the Assignment, ConsumerCoordinator will request re-join if the revoked partitions list is not empty. 6. Updated ConsumerCoordinatorTest accordingly. Also found a minor bug in MetadataUpdate that removed topic would still be retained with null value of num.partitions. 6. Updated a few other flaky tests that are exposed due to this change. Reviewers: John Roesler <vvcephei@users.noreply.github.com>, A. Sophie Blee-Goldman <sophie@confluent.io>, Jason Gustafson <jason@confluent.io>
Author: Mickael Maison <mickael.maison@gmail.com> Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>, Viktor Somogyi <viktorsomogyi@gmail.com> Closes #7038 from mimaison/KAFKA-8598
The introduction of KIP-480: Sticky Producer Partitioner had the side effect that generateAndProduceMessages can often write messages to a lower number of partitions to improve batching. testDescribeLogDirsRequest (and potentially other tests) relies on the messages being written somewhat uniformly to the topic partitions. We fix the issue by including a monotonically increasing key in the produced messages. I also included a couple of minor clean-ups I noticed while debugging the issue. The test failed very frequently when executed locally before the change and it passed 100 times consecutively after the change. Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>
We recently introduced a bunch of flaky tests in the AdminClientIntegrationTest. These tests are failing very frequently. We should ignore the tests in order to make the build stable until we have a fix. Reviewers: Ismael Juma <ismael@juma.me.uk>
Reviewers: Boyang Chen <boyang@confluent.io>, Bill Bejeck <bill@confluent.io>
Connector validation fails if an alias is used for the converter since the validation for that is done via `ConfigDef.validateAll(...)`, which in turn invokes `Class.forName(...)` on the alias. Even though the class is successfully loaded by the DelegatingClassLoader, some Java implementations will refuse to return a class from `Class.forName(...)` whose name differs from the argument provided. This commit alters `ConfigDef.parseType(...)` to first invoke `ClassLoader.loadClass(...)` on the class using our class loader in order to get a handle on the actual class object to be loaded, then invoke `Class.forName(...)` with the fully-qualified class name of the to-be-loaded class and return the result. The invocation of `Class.forName(...)` is necessary in order to allow static initialization to take place; simply calling `ClassLoader.loadClass(...)` is insufficient. Also corrected a unit test that relied upon the old behavior. Author: Chris Egerton <chrise@confluent.io> Reviewers: Robert Yokota <rayokota@gmail.com>, Randall Hauch <rhauch@gmail.com>
…ted (#7193) Avoids calling both `containsKey` and `get` from isMuted, when only a single get is necessary. Also avoid calling `remove` unless necessary. This could be a reduction of map operations from 3 to 1. isMuted showed up as a hotspot in profiling when using the producer with high numbers of partitions. Reviewers: Ismael Juma <ismael@juma.me.uk>
Since `Metrics` was constructed with `enableExpiration=false`, this was not a source of flakiness given the current implementation. This could change in the future, so good to follow the class contract. Included a few clean-ups with regards to redundant casts and type parameters as well as usage of try with resources for inline usage of `Metrics`. Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>
…7173) The assignReplicasToBrokers method has helpful but a large unformatted javadoc comment that results in a big blob in generated html. This change formats the comment so that generated javadoc is nice. Reviewers: Stanislav Kozlovski <familyguyuser192@windowslive.com>, Jason Gustafson <jason@confluent.io>
Reviewers: Colin P. McCabe <cmccabe@apache.org>, Lucas Bradstreet <lucasbradstreet@gmail.com>
Corrected the AbstractHerder to correctly identify task configs that contain variables for externalized secrets. The original method incorrectly used `matcher.matches()` instead of `matcher.find()`. The former method expects the entire string to match the regex, whereas the second one can find a pattern anywhere within the input string (which fits this use case more correctly). Added unit tests to cover various cases of a config with externalized secrets, and updated system tests to cover case where config value contains additional characters besides secret that requires regex pattern to be found anywhere in the string (as opposed to complete match). Author: Arjun Satish <arjun@confluent.io> Reviewer: Randall Hauch <rhauch@gmail.com>
) When calling readLogToEnd(), the KafkaBasedLog worker thread should catch TimeoutException and log a warning, which can occur if brokers are unavailable, otherwise the worker thread terminates. Includes an enhancement to MockConsumer that allows simulating exceptions not just when polling but also when querying for offsets, which is necessary for testing the fix. Author: Paul Whalen <pgwhalen@gmail.com> Reviewers: Randall Hauch <rhauch@gmail.com>, Arjun Satish <arjun@confluent.io>, Ryanne Dolan <ryannedolan@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)