Skip to content

Kafka-15126: Range queries to accept null lower and upper bounds#13987

Closed
Cerchie wants to merge 88 commits intoapache:trunkfrom
Cerchie:KAFKA-15126
Closed

Kafka-15126: Range queries to accept null lower and upper bounds#13987
Cerchie wants to merge 88 commits intoapache:trunkfrom
Cerchie:KAFKA-15126

Conversation

@Cerchie
Copy link
Contributor

@Cerchie Cerchie commented Jul 10, 2023

Change in response to KIP-941.

Changes line 57 in the RangeQuery class file from:

public static <K, V> RangeQuery<K, V> withRange(final K lower, final K upper) {
    return new RangeQuery<>(Optional.of(lower), Optional.of(upper));
}

to

public static <K, V> RangeQuery<K, V> withRange(final K lower, final K upper) {
     return new RangeQuery<>(Optional.ofNullable(lower), Optional.ofNullable(upper));
 }

Testing strategy:

Since null values can now be entered in RangeQuerys in order to receive full scans, I changed the logic defining query starting at line 1085 in IQv2StoreIntegrationTest.java from:

        final RangeQuery<Integer, V> query;
        if (lower.isPresent() && upper.isPresent()) {
            query = RangeQuery.withRange(lower.get(), upper.get());
        } else if (lower.isPresent()) {
            query = RangeQuery.withLowerBound(lower.get());
        } else if (upper.isPresent()) {
            query = RangeQuery.withUpperBound(upper.get());
        } else {
            query = RangeQuery.withNoBounds();
        }

to

query = RangeQuery.withRange(lower.orElse(null), upper.orElse(null));

because different combinations of isPresent() in the bounds is no longer necessary.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Copy link
Member

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @Cerchie - overall, this looks good to me.
I think it would be good to update the Javadoc on the withRange(lower, upper) method to briefly describe the behavior if one or both of the parameters are null

Copy link
Member

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

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

LGTM

@bbejeck
Copy link
Member

bbejeck commented Jul 25, 2023

@Cerchie since it's been a couple of weeks for this PR can you rebase this with trunk?
Pending the related tests passing we should be able to merge

ping @vvcephei for a quick second look

hudeqi and others added 22 commits July 25, 2023 13:06
Reviewers: Divij Vaidya <diviv@amazon.com>

---------

Co-authored-by: Deqi Hu <deqi.hu@shopee.com>
…pache#13661)

Various formatting fixes in the config docs.

Reviewers: Bill Bejeck <bbejeck@apache.org>
…lient retriable exceptions in AbstractWorkerSourceTask (apache#13955)

Reviewers: Sagar Rao <sagarmeansocean@gmail.com>, Chris Egerton <chrise@aiven.io>
…tor (apache#13946)

Reviewed-by: Greg Harris <greg.harris@aiven.io>
Reviewers: Divij Vaidya <diviv@amazon.com>

---------

Co-authored-by: Damon Xie <damon.xie@zoom.us>
…nnelManager (apache#13988)

Reviewers: David Arthur <mumrah@gmail.com>
…iner is empty (apache#13974)

Reviewers: Divij Vaidya <diviv@amazon.com>
… status topic (apache#13453)

During fast consecutive rebalances where a task is revoked from one worker and assigned to another one, it has been observed that there is a small time window and thus a race condition during which a RUNNING status record in the new generation is produced and is immediately followed by a delayed UNASSIGNED status record belonging to the same or a previous generation before the worker that sends this message reads the RUNNING status record that corresponds to the latest generation.
Although this doesn't inhibit the actual execution of tasks, it reports an incorrect status for those tasks(i.e UNASSIGNED). If the users have setup some kind of monitoring on tasks status then this could lead to false alarms for example.
This fix addresses this problem by checking if a status message is stale after reading it and updates it's status only when it is safe to. 

Reviewers: Lucent-Wong <manchesterfans@live.cn>, Chris Egerton <chrise@aiven.io>, Yash Mayya <yash.mayya@gmail.com>, Konstantine Karantasis <k.karantasis@gmail.com>
…he#13275)

KAFKA-14522 Rewrite and Move of RemoteIndexCache to storage module.
Cleanedup index file suffix usages and other minor cleanups

Reviewers: Jun Rao <junrao@gmail.com>, Ismael Juma <ismael@juma.me.uk>, Luke Chen <showuon@gmail.com>, Divij Vaidya <diviv@amazon.com>, Kamal Chandraprakash<kamal.chandraprakash@gmail.com>, Jorge Esteban Quilcate Otoya <quilcate.jorge@gmail.com>
Reviewers: Matthias J. Sax <matthias@confluent.io>
Reviewers: Luke Chen <showuon@gmail.com>
…stem tests (apache#13859)


Reviewers: Luke Chen <showuon@gmail.com>, Christo Lolov  <christololov@gmail.com>
…d insights (apache#13676)


Reviewers: Mickael Maison <mickael.maison@gmail.com>, Ismael Juma <ismael@juma.me.uk>, Divij Vaidya <diviv@amazon.com>
…#13963)

This patch adds the session timeout and the revocation timeout to the new consumer group protocol.

Reviewers: Calvin Liu <caliu@confluent.io>, Jeff Kim <jeff.kim@confluent.io>, Justine Olshan <jolshan@confluent.io>
…le in MBean server (apache#13995)

In JmxTool.scala, we will wait till all the object names are available from MBean server. But in the newer version, we only wait for subset of object names. Due to this, we may not enforce wait option and prematurely return the result if the objects are not yet registered in MBean sever.

Reviewers: Luke Chen <showuon@gmail.com>, Federico Valeri <fvaleri@redhat.com>
…ning as unit tests (apache#13973)

Reviewers: Divij Vaidya <diviv@amazon.com>
Reviewers: Sagar Rao <sagarmeansocean@gmail.com>, Yash Mayya <yash.mayya@gmail.com>, 
Sudesh Wasnik <swasnik@confluent.io>, Chris Egerton <chrise@aiven.io>
Catch any exceptions that escape the processing logic
inside TaskExecutors and record them in the TaskManager.
Make sure the TaskExecutor survives, but the task is
unassigned. Add a method to TaskManager to drain the
exceptions. The aim here is that the polling thread will
drain the exceptions to be able to execute the
uncaught exception handler, abort transactions, etc.

Reviewer: Bruno Cadonna <cadonna@apache.org>
…che#13703)

Standardize controller log4j output for replaying important records. The log message should include
word "replayed" to make it clear that this is a record replay. Log the replay of records for ACLs,
client quotas, and producer IDs, which were previously not logged. Also fix a case where we weren't
logging changes to broker registrations.

AclControlManager, ClientQuotaControlManager, and ProducerIdControlManager didn't previously have a
log4j logger object, so this PR adds one. It also converts them to using Builder objects. This
makes junit tests more readable because we don't need to specify paramaters where the test can use
the default (like LogContexts).

Throw an exception in replay if we get another TopicRecord for a topic which already exists.

Example log messages:
  INFO [QuorumController id=3000] Replayed a FeatureLevelRecord setting metadata version to 3.6-IV0
  DEBUG [QuorumController id=3000] Replayed a ZkMigrationStateRecord which did not alter the state from NONE.
  INFO [QuorumController id=3000] Replayed BrokerRegistrationChangeRecord modifying the registration for broker 0: BrokerRegistrationChangeRecord(brokerId=0, brokerEpoch=3, fenced=-1, inControlledShutdown=0)
  INFO [QuorumController id=3000] Replayed ClientQuotaRecord for ClientQuotaEntity(entries={user=testkit}) setting request_percentage to 0.99.

Reviewers: Divij Vaidya <diviv@amazon.com>, Ron Dagostino <rndgstn@gmail.com>, David Arthur <mumrah@gmail.com>
yashmayya and others added 28 commits July 25, 2023 13:06
…ools dependency from connect-runtime (apache#13313)

Reviewers: Ismael Juma <ismael@juma.me.uk>
Part of KIP-925.

Reviewers: Matthias J. Sax <matthias@confluent.io>
Added tests for metrics:
1. RemoteLogReaderTaskQueueSize
2. RemoteLogReaderAvgIdlePercent
3. RemoteLogManagerTasksAvgIdlePercent

Also, added tests for OffsetOutOfRangeException will be thrown while reading logs

Reviewers: Christo Lolov <christololov@gmail.com>, Satish Duggana <satishd@apache.org>
Fixing the build failure caused by the earlier commit apache@27ea025 


```
[Error] /Users/satishd/repos/apache-kafka/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:3526:77: the result type of an implicit conversion must be more specific than Object
[Error] /Users/satishd/repos/apache-kafka/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:3530:70: the result type of an implicit conversion must be more specific than Object
[Warn] /Users/satishd/repos/apache-kafka/core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala:23:21: imported `QuorumTestHarness` is permanently hidden by definition of object QuorumTestHarness in package server
[Warn] /Users/satishd/repos/apache-kafka/core/src/test/scala/unit/kafka/server/ServerGenerateClusterIdTest.scala:29:21: imported `QuorumTestHarness` is permanently hidden by definition of object QuorumTestHarness in package server
[Error] /Users/satishd/repos/apache-kafka/core/src/test/scala/unit/kafka/utils/TestUtils.scala:1438:15: ambiguous reference to overloaded definition,
both method doReturn in class Mockito of type (x$1: Any, x$2: Object*)org.mockito.stubbing.Stubber
and  method doReturn in class Mockito of type (x$1: Any)org.mockito.stubbing.Stubber
match argument types (kafka.log.UnifiedLog)
```

Reviewers: Luke Chen <showuon@gmail.com>
Reviewers: Divij Vaidya <diviv@amazon.com>
Adding the value 20 to the JDK version that can build Apache Kafka into README.md

Reviewers: Divij Vaidya <diviv@amazon.com>
Reviewers: Mickael Maison <mickael.maison@gmail.com>, Federico Valeri <fedevaleri@gmail.com>
…ion and offset in sink records (apache#14024)

Reviewers: Greg Harris <greg.harris@aiven.io>, Chris Egerton <chrise@aiven.io>
…taleMemberEpochException error (apache#14046)

This patch does a few things:
1) It introduces version 9 of the OffsetCommit API. This new version has no schema changes but it can return a StaleMemberEpochException if the new consumer group protocol is used. Note the use of `"latestVersionUnstable": true` in the request schema. This means that this new version is not available yet unless activated.
2) It renames the `generationId` field in the request to `GenerationIdOrMemberEpoch`. This is backward compatible change.
3) It introduces the new StaleMemberEpochException error.
4) It does a minor refactoring in OffsetCommitRequest class.

Reviewers: Jeff Kim <jeff.kim@confluent.io>, David Arthur <mumrah@gmail.com>, Justine Olshan <jolshan@confluent.io>
This patch does a few things:
1) It introduces the `OffsetAndMetadata` class which hold the committed offsets in the group coordinator.
2) It adds methods to deal with OffsetCommit records to `RecordHelpers`.
3) It adds `MetadataVersion#offsetCommitValueVersion` to get the version of the OffsetCommit value record that should be used.

Reviewers: Jeff Kim <jeff.kim@confluent.io>, David Arthur <mumrah@gmail.com>, Justine Olshan <jolshan@confluent.io>
Reviewers: Mickael Maison <mickael.maison@gmail.com>
We will explicitly send an assignment change event to the background thread to invoke auto-commit if the group.id is configured. After updating the subscription state, a NewTopicsMetadataUpdateRequestEvent will also be sent to the background thread to update the metadata.

Co-authored-by: Kirk True <kirk@kirktrue.pro>
Reviewers: Jun Rao <junrao@gmail.com>
… consumer-manager/task (apache#14045)

Improved logging and docs on consumer manager/task call paths.

Reviewers: Luke Chen <showuon@gmail.com>, Satish Duggana <satishd@apache.org>
…pache#13773)

Fix the confusing error message in ImageWriterOptions

Reviewers: Luke Chen <showuon@gmail.com>, David Arthur <mumrah@gmail.com>
…ata cache (apache#14004)

KAFKA-15168: Handle overlapping remote log segments in RemoteLogMetadata cache

Reviewers: Satish Duggana <satishd@apache.org>, Viktor Nikitash <nikitashvictor@pdffiller.com>, Jorge Esteban Quilcate Otoya <quilcate.jorge@gmail.com>, Abhijeet Kumar <abhijeet.cse.kgp@gmail.com>
When creating a verification state entry, we also store sequence and epoch. On subsequent requests, we will take the latest epoch seen and the earliest sequence seen. That way, if we try to append a sequence after the earliest seen sequence, we can block that and retry. This addresses potential OutOfOrderSequence loops caused by errors during verification (coordinator loading, timeouts, etc).

Reviewers:  David Jacot <david.jacot@gmail.com>,  Artem Livshits <alivshits@confluent.io>
…oker restart (apache#13707)

Dynamic overrides for the producer ID expiration config are not picked up on broker restart in Zookeeper mode. Based on the integration test, this does not apply to KRaft mode.

Adds a broker restart that fails without the corresponding KafkaConfig change.

Reviewers: Justine Olshan <jolshan@confluent.io>
…pache#14010)

Implement some of the metrics from KIP-938: Add more metrics for
measuring KRaft performance.

Add these metrics to QuorumControllerMetrics:
    kafka.controller:type=KafkaController,name=TimedOutBrokerHeartbeatCount
    kafka.controller:type=KafkaController,name=EventQueueOperationsStartedCount
    kafka.controller:type=KafkaController,name=EventQueueOperationsTimedOutCount
    kafka.controller:type=KafkaController,name=NewActiveControllersCount

Create LoaderMetrics with these new metrics:
    kafka.server:type=MetadataLoader,name=CurrentMetadataVersion
    kafka.server:type=MetadataLoader,name=HandleLoadSnapshotCount

Create SnapshotEmitterMetrics with these new metrics:
    kafka.server:type=SnapshotEmitter,name=LatestSnapshotGeneratedBytes
    kafka.server:type=SnapshotEmitter,name=LatestSnapshotGeneratedAgeMs

Reviewers: Ron Dagostino <rndgstn@gmail.com>
…k thread to the sink task thread (apache#14079)

Reviewers: Chris Egerton <chrise@aiven.io>
Reviewers: Mickael Maison <mickael.maison@gmail.com>
@Cerchie
Copy link
Contributor Author

Cerchie commented Aug 1, 2023

closing due to merge issue, refer to #14137

@Cerchie Cerchie closed this Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.