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

Missing the ConsumerSeekAware interface #812

Merged
merged 41 commits into from
Aug 22, 2023

Conversation

guillermocalvo
Copy link
Contributor

@guillermocalvo guillermocalvo commented Aug 14, 2023

Added two new mechanisms to allow for immediate/deferred Kafka seek operations:

  • New interface ConsumerSeekAware allows consumers to react when the set of partitions assigned to the consumer changes.
@KafkaListener
class Example implements ConsumerSeekAware {
    /* ... */
    @Override 
    public void onPartitionsAssigned(Collection<TopicPartition> partitions, KafkaSeeker seeker) {
        seeker.seekToBeginning(partitions).forEach(seeker::perform);
    }
}
  • New interface KafkaSeekOperations can be injected into consumer methods so that seek operations can be performed when/if the method completes successfully.
@KafkaListener
class Example {
    static final TopicPartition TP = new TopicPartition("foo", 0);
    @Topic("foo")
    public void consume(String message, KafkaSeekOperations ops) {
        /* ... */
        ops.defer(ops.seekToEnd(TP));
    }
}

@guillermocalvo guillermocalvo added the type: enhancement New feature or request label Aug 14, 2023
@guillermocalvo guillermocalvo self-assigned this Aug 14, 2023
@guillermocalvo guillermocalvo linked an issue Aug 14, 2023 that may be closed by this pull request
@guillermocalvo guillermocalvo force-pushed the 46-missing-the-consumerseekaware-interface branch from 92b9470 to 05ba997 Compare August 15, 2023 10:02
@guillermocalvo guillermocalvo force-pushed the 46-missing-the-consumerseekaware-interface branch from 05ba997 to 12debce Compare August 15, 2023 10:23
@guillermocalvo guillermocalvo marked this pull request as ready for review August 15, 2023 10:32
Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

Add tests for the code samples in test-suite, test-suite-groovy and `test-suite-kotlin

@sdelamo
Copy link
Contributor

sdelamo commented Aug 18, 2023

@guillermocalvo I merged master into the branch but the test fails for me. can you investigate? Sorry 😬

@guillermocalvo
Copy link
Contributor Author

guillermocalvo commented Aug 18, 2023

@sdelamo

@guillermocalvo I merged master into the branch but the test fails for me. can you investigate? Sorry 😬

You removed MY_KAFKA.stop(), which allowed the tests to run in isolation. Do you really want that removed or, can we simply put it back?

@sdelamo
Copy link
Contributor

sdelamo commented Aug 18, 2023

@sdelamo

@guillermocalvo I merged master into the branch but the test fails for me. can you investigate? Sorry 😬

You removed MY_KAFKA.stop(), which allowed the tests to run in isolation. Do you really want that removed or, can we simply put it back?

ping @timyates 🖕. if possible it would be good to use the same Kafka instance for them.

@sdelamo
Copy link
Contributor

sdelamo commented Aug 18, 2023

You removed MY_KAFKA.stop(), which allowed the tests to run in isolation. Do you really want that removed or, can we simply put it back?

I removed it because MY_KAFKA property no longer exists since #824.

@sdelamo
Copy link
Contributor

sdelamo commented Aug 18, 2023

@wetted changed test-suite, test-suite-groovy and test-suite-kotlin to use Test Resources

#825

I have merged master into this branch and remove AbstractKafkaTest

However, there are failures in test-suite, test-suite-groovy and test-suite-kotlin.

@guillermocalvo
Copy link
Contributor Author

@wetted changed test-suite, test-suite-groovy and test-suite-kotlin to use Test Resources

#825

I have merged master into this branch and remove AbstractKafkaTest

However, there are failures in test-suite, test-suite-groovy and test-suite-kotlin.

@sdelamo Those classes were there to illustrate section Running Kafka while testing and developing of the guide.

@wetted
Copy link
Contributor

wetted commented Aug 18, 2023

@sdelamo

As @guillermocalvo notes AbstractKafkaTest, and MyTest which uses it should remain in test-suite-* for this part of the guide. https://micronaut-projects.github.io/micronaut-kafka/latest/guide/#kafkaDockerized

But AbstractKafkaTest should be not used for anything else because I'm now using test-resources for everything else, as recommended by that same section of the guide.

see #826 as an example

I am updating other sections in the guide to do the same (i.e. multi-lang snippets with test-resources tests).

@guillermocalvo
Copy link
Contributor Author

@sdelamo I updated the tests to use test-resources. BTW SonarCloud is in the wrong again.

@sdelamo
Copy link
Contributor

sdelamo commented Aug 22, 2023

ping @dstepanov

}

@NonNull
private Optional<Boolean> performForZeroOffset(@NonNull KafkaSeekOperation operation,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use just nullable boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok so I did what you requested but then Sonar was complaining about "null should not be returned from a Boolean method" and I had to revert the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

@sonarcloud
Copy link

sonarcloud bot commented Aug 22, 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

94.6% 94.6% Coverage
0.0% 0.0% Duplication

@sdelamo sdelamo merged commit 4dfe2f5 into master Aug 22, 2023
7 checks passed
@sdelamo sdelamo deleted the 46-missing-the-consumerseekaware-interface branch August 22, 2023 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Missing the ConsumerSeekAware interface
4 participants