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

Skyeden-3020 | retransimission with detached consumers #1938

Merged
merged 11 commits into from
Jan 10, 2025

Conversation

MarcinBobinski
Copy link
Collaborator

@MarcinBobinski MarcinBobinski commented Dec 16, 2024

Current retransmission solution requires that the Hermes subscription is active and has consumers attached to the consumer group. If this condition is not met, retransmission will not occur.

This PR introduces the ability to perform retransmission by management instances (via offset changes on the consumer group) when the subscription is suspended and no consumers are attached to the group. Additionally, it adds validation to check the Hermes subscription state and the consumer group itself before performing retransmission.

@@ -4,6 +4,7 @@
import { useDialog } from '@/composables/dialog/use-dialog/useDialog';
import { useI18n } from 'vue-i18n';
import ConfirmationDialog from '@/components/confirmation-dialog/ConfirmationDialog.vue';
import LoadingSpinner from '@/components/loading-spinner/LoadingSpinner.vue';
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

retransmit: [fromDate: string];
skipAllMessages: [];
retransmit: [fromDate: string, onComplete: () => void];
skipAllMessages: [onComplete: () => void];
Copy link
Contributor

Choose a reason for hiding this comment

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

is this necessary to have onComplete here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, now that I think about it, a cleaner way would be to eliminate that and lift the state to the parent component.

* The consumers are responsible for moving their local offsets(KafkaConsumer::seek method) as well as committed ones on Kafka (KafkaConsumer::commitSync method).
* When the subscription is SUSPENDED, the management instance changes the commited offsets on kafka on its own (AdminClient::alterConsumerGroupOffsets).
* There is no active consumer to notify in that case.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the explanation 🙇

cluster.getClusterName(),
cluster.indicateOffsetChange(topic, subscriptionName, timestamp, dryRun)));

if (!dryRun) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do You know why dryRun flag was used and is this needed?

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 have no clue at all, I haven't decided to remove it because i don't want to break anything :P

offsets -> cluster.indicateOffsetChange(topic, subscriptionName, offsets)));

logger.info(
"Starting offsets move for subscription {}. Requested by {}. Retransmission offsets: {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: maybe "Starting moving offsets" would sound better? let me know what do You think about it


OffsetNotFoundException(String message) {
public OffsetNotFoundException(String message) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed to be public?

MarcinBobinski and others added 4 commits January 8, 2025 11:19
…simissionWithDetachedConsumers

# Conflicts:
#	hermes-console/src/views/subscription/SubscriptionView.spec.ts
#	hermes-console/src/views/subscription/SubscriptionView.vue
…simissionWithDetachedConsumers

# Conflicts:
#	hermes-management/src/main/java/pl/allegro/tech/hermes/management/config/kafka/KafkaConfiguration.java
#	hermes-management/src/main/java/pl/allegro/tech/hermes/management/infrastructure/kafka/MultiDCAwareService.java
#	hermes-management/src/main/java/pl/allegro/tech/hermes/management/infrastructure/kafka/service/BrokersClusterService.java
"Starting moving offsets for subscription {}. Requested by {}. Retransmission offsets: {}",
topic.getQualifiedName() + "$" + subscriptionName,
requester.getUsername(),
brokerPartitionOffsets);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it was before your change but this will not print partitions but a java object refrence: the PartitionOffset has no toString() method overriden ;) Could you fix that in the PR? ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

example from tests

Successfully moved offsets for retransmission of subscription TopicBuilderGroup1.TopicBuilderTopic2$subscription1. Requested by user: test-user. Retransmission offsets: {primary-dc=[pl.allegro.tech.hermes.common.kafka.offset.PartitionOffset@e057ca5d, pl.allegro.tech.hermes.common.kafka.offset.PartitionOffset@e057ca7c]}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch 👍

faderskd
faderskd previously approved these changes Jan 9, 2025
szczygiel-m
szczygiel-m previously approved these changes Jan 9, 2025
@MarcinBobinski MarcinBobinski dismissed stale reviews from szczygiel-m and faderskd via 5780ef7 January 9, 2025 14:45
@szczygiel-m szczygiel-m merged commit f551c5b into master Jan 10, 2025
13 checks passed
@szczygiel-m szczygiel-m deleted the SKYEDEN-3020-RetransimissionWithDetachedConsumers branch January 10, 2025 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants