Skip to content

Conversation

@jozefbakus
Copy link
Collaborator

@jozefbakus jozefbakus commented Feb 23, 2023

Implements: #789

@jozefbakus jozefbakus added enhancement New feature or request UX Something concerning UX backend labels Feb 23, 2023
@jozefbakus jozefbakus self-assigned this Feb 23, 2023
kevinwallimann
kevinwallimann previously approved these changes Mar 6, 2023
Copy link
Collaborator

@filiphornak filiphornak left a comment

Choose a reason for hiding this comment

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

The code is well-written, at least the scala part. However, there is one inconsistency when reading the last commit file, which may throw an exception even when the method returns the Try monad.

Also, reading offsets from kafka and the latest checkpoint file are not parallelized due to the current implementation of the kafka cache. However, this would require a proper consumer pool implementation because calls on ordinary kafka consumer are not thread-safe or finding alternative how to request offsets from kafka without a consumer.

def getBeginningEndOffsets(topic: String, consumerProperties: Properties): BeginningEndOffsets = {
BeginningEndOffsets(
topic,
getOffsets(topic, consumerProperties, BeginningOffsets),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a suggestion for future projects because it would require reworking or unification of the entire architecture. I noticed that some services use Futures as output, and some don't even when they are blocking.

case class Offset(beginning: Long, end: Long)

def getBeginningEndOffset(topic: String, consumerProperties: Properties): Future[Map[Long, Offset]] =
  for {
    kafkaConsumer   <- consumerPool.getConsumer(consumerProperties)
    parts           <- listTopicPartitions(kafkaConsumer, topic)
    futureBeginning  = getBeginningOffsets(kafka, parts)
    futureEnd        = getEndOffsets(kafka, parts)
    begin           <- futureBegin
    end             <- futureEnd
  } yield begin.map { case (part, bOff) => part -> Offset(bOff, end(part)) }  // This might be done in safer manner

This might improve performance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with you. Please create an issue for this one. With this PR I just follow style that was already introduced

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will do!

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 9, 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 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@filiphornak filiphornak left a comment

Choose a reason for hiding this comment

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

I'm pleased with this explanation and your changes, so I approve this PR. Also, I will create issues for the things we discussed earlier.

def getBeginningEndOffsets(topic: String, consumerProperties: Properties): BeginningEndOffsets = {
BeginningEndOffsets(
topic,
getOffsets(topic, consumerProperties, BeginningOffsets),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will do!

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

Labels

backend enhancement New feature or request UX Something concerning UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants