Skip to content

Conversation

@klevy-toast
Copy link
Contributor

@klevy-toast klevy-toast commented Nov 3, 2025

Fixes #164

Motivation

Update namespace / topic policy defaults to -1 indicating unset. Add logic to remove configs when unset.

Modifications

  • Update pulsar-client-go to latest snapshot version
  • Add logic to remove specific configs when they have a -1 value (unset)
  • Update the default in relevant configs to be -1

Verifying this change

  • Make sure that the change passes the CI checks.
    • Seems like there's some flakiness with schema import test, but other tests are passing

(Please pick either of the following options)

This change added tests and can be verified as follows:

  • Added new test files that verify the -1 default behavior

Documentation

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

@klevy-toast:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions github-actions bot added the doc-info-missing This pr needs to mark a document option in description label Nov 3, 2025
@klevy-toast klevy-toast force-pushed the fix/policy-default-unset branch from 19e3e01 to b02f5b1 Compare November 3, 2025 23:57
@github-actions github-actions bot removed the doc-info-missing This pr needs to mark a document option in description label Nov 4, 2025
@github-actions
Copy link

github-actions bot commented Nov 4, 2025

@klevy-toast:Thanks for providing doc info!

@github-actions github-actions bot added the doc This pr contains a document label Nov 4, 2025
@klevy-toast klevy-toast force-pushed the fix/policy-default-unset branch from e84b619 to 056edbc Compare November 4, 2025 19:28
@klevy-toast klevy-toast marked this pull request as ready for review November 4, 2025 22:39
@klevy-toast klevy-toast requested a review from a team as a code owner November 4, 2025 22:39
@klevy-toast klevy-toast changed the title Fix/policy default unset Make namespace/topic policies unset by default Nov 4, 2025
@klevy-toast
Copy link
Contributor Author

@maxsxu @freeznet could you take a look please? Thank you

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR changes the default values for various topic and namespace configuration parameters from 0 to -1 to better distinguish between "explicitly set to 0" (unlimited/disabled) and "use namespace/broker defaults" (unset). It introduces removal operations for when values are set to -1, adds comprehensive test coverage for default behavior, and updates the Pulsar client dependency.

Key changes:

  • Default values changed from 0 to -1 for max_consumers, max_producers, message_ttl_seconds, max_unacked_messages_per_consumer, max_unacked_messages_per_subscription, msg_publish_rate, and byte_publish_rate in topic resources, and similar parameters in namespace resources
  • Update logic enhanced to call Remove* API methods when values are -1
  • New test files verify default, removal, and explicit zero-value behaviors

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 17 comments.

Show a summary per file
File Description
pulsar/resource_pulsar_topic.go Updated default values to -1, added descriptions, enhanced read/update logic for removal operations
pulsar/resource_pulsar_namespace.go Updated default values to -1 and added descriptions for namespace config parameters
pulsar/resource_pulsar_topic_test.go Updated test assertions from "0" to "-1" and added new test step for partial configuration
pulsar/resource_pulsar_topic_defaults_test.go New test file covering default behavior, parameter removal, and explicit zero values for topics
pulsar/resource_pulsar_namespace_defaults_test.go New test file covering default behavior, parameter removal, and explicit zero values for namespaces
go.mod Updated pulsar-client-go dependency version
go.sum Updated checksums for new dependency versions
Comments suppressed due to low confidence (1)

pulsar/resource_pulsar_namespace.go:226

  • The validateGtEq0 function only allows values >= 0, but with the new default of -1, this validation will reject the default value. The validation function needs to be updated to allow -1 (e.g., validateGtEqNeg1 that accepts values >= -1), or the Default should be removed and handled differently.
							Optional: true,

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1591 to +1595
if !isIgnorableTopicPolicyError(err) {
errs = errors.Wrap(errs, fmt.Sprintf("RemovePublishRate error: %v", err))
} else {
return backoff.Permanent(fmt.Errorf("ERROR_UPDATE_PUBLISH_RATE: RemovePublishRate: %w", err))
}
Copy link

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

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

The error handling logic is inverted. When isIgnorableTopicPolicyError returns false (non-ignorable error), the code wraps the error but continues. When it returns true (ignorable error), it returns a permanent error. This should be: if the error is NOT ignorable, return permanent error; if it IS ignorable, wrap and continue.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not change this error handling logic so I'm unsure on the original intention.

maxsxu
maxsxu previously approved these changes Nov 8, 2025
Copy link
Member

@freeznet freeznet left a comment

Choose a reason for hiding this comment

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

lgtm, but could you please help address the changes to docs as well, thanks.

@klevy-toast
Copy link
Contributor Author

lgtm, but could you please help address the changes to docs as well, thanks.

Just ran the doc generation again!

@klevy-toast
Copy link
Contributor Author

@freeznet @maxsxu Let me know of any further steps to get this merged, thanks!

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

Labels

doc This pr contains a document

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent defaults for override settings -- no way to unset config properties

4 participants