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

Fixing Failure when creating a cross-partition subscription if the default partition id is not null #6687

Conversation

epeartree
Copy link
Collaborator

@epeartree epeartree commented Feb 6, 2025

Background:

  • The default value of the ID for the default partition is null;
  • clients can provide another value for the default partition id through the partitionSettings.setDefaultPartitionId

Root cause of the issue:
invocation of RequestPartitionId.isDefaultParttition() only considers that the default partition id is and should be null;

What was done:

  • added failing test;
  • added api to an existing helper service to perform default partition check against the client configured defaultPartition Id (PartitionSettings);

Closes #6686

Copy link

github-actions bot commented Feb 6, 2025

Formatting check succeeded!

@TipzCM
Copy link
Collaborator

TipzCM commented Feb 6, 2025

Request: can you put a logline in SubscriptionCanonicalizer to log something if no subscriptions match?

Just to make tracking bugs down here a bit easier next time?

Copy link
Collaborator

@TipzCM TipzCM left a comment

Choose a reason for hiding this comment

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

Also, please add a log line in SubscriptionMatchingSubscriber so that when no subscriptions match, we have some loglines (warnings is my suggestion, but can ask in design)

Copy link

codecov bot commented Feb 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (rel_8_0@ce0c41c). Learn more about missing BASE report.

Additional details and impacted files
@@            Coverage Diff             @@
##             rel_8_0    #6687   +/-   ##
==========================================
  Coverage           ?   83.59%           
  Complexity         ?    28647           
==========================================
  Files              ?     1798           
  Lines              ?   111328           
  Branches           ?    13979           
==========================================
  Hits               ?    93060           
  Misses             ?    12269           
  Partials           ?     5999           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

peartree and others added 4 commits February 10, 2025 07:23
…creating-a-cross-partition-subscription-if-the-default-partition-id-is-not-null
…ion-subscription-if-the-default-partition-id-is-not-null
…ion-subscription-if-the-default-partition-id-is-not-null
@epeartree epeartree enabled auto-merge (squash) February 10, 2025 23:37
@epeartree epeartree merged commit a579d8f into rel_8_0 Feb 11, 2025
67 checks passed
@epeartree epeartree deleted the 6686-failure-when-creating-a-cross-partition-subscription-if-the-default-partition-id-is-not-null branch February 11, 2025 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants