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.

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

Messages API v2 #3537

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Messages API v2 #3537

wants to merge 23 commits into from

Conversation

iliax
Copy link
Contributor

@iliax iliax commented Mar 22, 2023

  • Breaking change? (if so, please describe the impact and migration path for existing application instances)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)

  • No need to
  • Manually (please, describe, if necessary)
  • Unit checks
  • Integration checks
  • Covered by existing automation

Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. ENVIRONMENT VARIABLES)
  • My changes generate no new warnings (e.g. Sonar is happy)
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

Check out Contributing and Code of Conduct

A picture of a cute animal (not mandatory but encouraged)

@iliax iliax requested review from a team and Haarolean as code owners March 22, 2023 19:19
@github-actions github-actions bot added the status/triage Issues pending maintainers triage label Mar 22, 2023
@iliax iliax changed the title Messages API v2 WIP: Messages API v2 Mar 23, 2023
@sonarcloud
Copy link

sonarcloud bot commented Mar 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@iliax iliax linked an issue Mar 24, 2023 that may be closed by this pull request
iliax added 4 commits April 24, 2023 11:45
…4_messagesApiV2

� Conflicts:
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/controller/MessagesController.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/TailingEmitter.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/service/MessagesService.java
�	kafka-ui-api/src/test/java/com/provectus/kafka/ui/service/RecordEmitterTest.java
@iliax iliax changed the title WIP: Messages API v2 Messages API v2 Apr 24, 2023
@sonarcloud
Copy link

sonarcloud bot commented Apr 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Contributor

@Haarolean Haarolean left a comment

Choose a reason for hiding this comment

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

Looks clean, as a suggestion let's ask frontend folks to take a look at the API, if it's understandable enough for them

public static final int MAX_SIZE = 10_000;

private final Cache<String, Cursor> cursorsCache = CacheBuilder.newBuilder()
.maximumSize(MAX_SIZE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we add expireAfterAccess and/or expireAfterWrite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is hard to define good properties for that. I think default policy (delete oldest when max size is reached) should work fine.

@@ -56,4 +57,8 @@ public boolean assignedPartitionsFullyPolled() {
return true;
}

public Set<TopicPartition> allTargetPartitions() {
return Sets.union(nonEmptyPartitions, emptyPartitions);
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't it better to calculate this in the constructor instead of doing so on every each method call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it only called in once and very light to created, so I guess its better to create it lazily

@Nullable Long timestamp,
@Nullable Offsets offsets) {

// one of properties will be null
Copy link
Contributor

Choose a reason for hiding this comment

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

should we assert that either is null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Haarolean Haarolean added type/enhancement En enhancement to an already existing feature scope/backend type/refactoring area/messages and removed status/triage Issues pending maintainers triage labels Jul 10, 2023
@Haarolean Haarolean added the impact/changelog A PR with changes which should be addressed in the changelog explicitly label Jul 10, 2023
iliax and others added 8 commits July 11, 2023 12:03
…4_messagesApiV2

� Conflicts:
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/controller/MessagesController.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/MessageFilters.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/service/MessagesService.java
�	kafka-ui-api/src/test/java/com/provectus/kafka/ui/service/MessagesServiceTest.java
…4_messagesApiV2

� Conflicts:
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/AbstractEmitter.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/ConsumingStats.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/ForwardRecordEmitter.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/TailingEmitter.java
�	kafka-ui-api/src/test/java/com/provectus/kafka/ui/service/RecordEmitterTest.java
…4_messagesApiV2

� Conflicts:
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/controller/MessagesController.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/AbstractEmitter.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/BackwardRecordEmitter.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/ConsumingStats.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/ForwardRecordEmitter.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/MessagesProcessing.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/OffsetsInfo.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/SeekOperations.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/emitter/TailingEmitter.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/serdes/ConsumerRecordDeserializer.java
�	kafka-ui-api/src/main/java/com/provectus/kafka/ui/service/MessagesService.java
�	kafka-ui-api/src/test/java/com/provectus/kafka/ui/service/RecordEmitterTest.java
@sonarcloud
Copy link

sonarcloud bot commented Aug 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

type: string
- name: offs
in: query
description: partition offsets to read from / to. Format is "p1:offset1,p2:offset2,...".
Copy link
Contributor Author

Choose a reason for hiding this comment

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

int64

public static final int MAX_SIZE = 10_000;

private final Cache<String, Cursor> cursorsCache = CacheBuilder.newBuilder()
.maximumSize(MAX_SIZE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is hard to define good properties for that. I think default policy (delete oldest when max size is reached) should work fine.

@Nullable Long timestamp,
@Nullable Offsets offsets) {

// one of properties will be null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -56,4 +57,8 @@ public boolean assignedPartitionsFullyPolled() {
return true;
}

public Set<TopicPartition> allTargetPartitions() {
return Sets.union(nonEmptyPartitions, emptyPartitions);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it only called in once and very light to created, so I guess its better to create it lazily

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/messages impact/changelog A PR with changes which should be addressed in the changelog explicitly scope/backend status/pending-frontend type/enhancement En enhancement to an already existing feature type/refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Messages v2: API overhaul
2 participants