Skip to content

Conversation

@freeznet
Copy link
Member

@freeznet freeznet commented Aug 4, 2025

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #

(or if this PR is one task of a github issue, please add Master Issue: #<xyz> to link to the master issue.)

Master Issue: #

Motivation

Explain here the context, and why you're making that change. What is the problem you're trying to solve.

Modifications

Describe the modifications you've done.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Documentation

Check the box below.

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)

@freeznet freeznet self-assigned this Aug 4, 2025
@freeznet freeznet requested review from a team as code owners August 4, 2025 14:14
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2025

@freeznet: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 Aug 4, 2025
@freeznet freeznet requested a review from Copilot August 5, 2025 17:59

This comment was marked as outdated.

Copy link
Member

@maxsxu maxsxu left a comment

Choose a reason for hiding this comment

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

@freeznet Wondering what's the disadvantages of achieving infinite retention policy by setting retentionTime and retentionSize to -1 compared to use new attribute?

@freeznet freeznet force-pushed the freeznet/infinite-retention-policy branch from acc04ab to 426f9f2 Compare August 7, 2025 03:35
@freeznet freeznet requested a review from Copilot August 7, 2025 03:43

This comment was marked as outdated.

@freeznet freeznet requested a review from Copilot August 7, 2025 04:41
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 adds support for infinite retention policies for PulsarTopic and PulsarNamespace resources by introducing the special value "-1" to represent infinite retention time and size.

  • Added utility functions for handling infinite quantities and durations
  • Enhanced admin client to properly handle infinite retention values when configuring Pulsar resources
  • Added comprehensive test coverage for infinite retention scenarios

Reviewed Changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pkg/utils/duration.go Enhanced Duration type to support "-1" as infinite duration value
pkg/utils/quantity.go Added new utility functions for handling infinite quantities
pkg/admin/impl.go Updated admin client to properly handle infinite retention in topic and namespace policies
api/v1alpha1/pulsartopic_types.go Updated field documentation to include infinite retention usage
config/crd/bases/resource.streamnative.io_pulsartopics.yaml Updated CRD with infinite retention documentation
docs/pulsar_topic.md Added comprehensive documentation for infinite retention configuration
docs/pulsar_namespace.md Added comprehensive documentation for infinite retention configuration
tests/utils/spec.go Added test utility functions for creating topics with various retention configurations
tests/operator/resources_test.go Added comprehensive integration tests for infinite retention functionality
pkg/utils/duration_test.go Added unit tests for Duration utility functions
pkg/utils/quantity_test.go Added unit tests for quantity utility functions
.licenserc.yaml Added .envrc to license exclusions

Comment on lines +44 to +45
// If can't convert to int64, use approximation
return int64(q.AsApproximateFloat64())
Copy link

Copilot AI Aug 7, 2025

Choose a reason for hiding this comment

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

Using AsApproximateFloat64() as a fallback could lead to precision loss for large quantities. Consider using q.Value() with proper scaling instead.

Suggested change
// If can't convert to int64, use approximation
return int64(q.AsApproximateFloat64())
// Use q.Value() to get the exact value and scale to bytes
bigVal := q.Value()
// Scale bigVal to bytes according to the quantity's format
// The Scale() method returns the exponent for the base-10 scale
scale := q.Scale()
// bytes = bigVal * 10^scale
scaled := new(big.Int).Set(bigVal)
if scale > 0 {
multiplier := new(big.Int).Exp(big.NewInt(10), big.NewInt(int64(scale)), nil)
scaled.Mul(scaled, multiplier)
} else if scale < 0 {
divisor := new(big.Int).Exp(big.NewInt(10), big.NewInt(int64(-scale)), nil)
scaled.Div(scaled, divisor)
}
// If the scaled value fits in int64, return it; otherwise, return math.MaxInt64
if scaled.IsInt64() {
return scaled.Int64()
}
// Overflow: return max int64 value
return int64(^uint64(0) >> 1)

Copilot uses AI. Check for mistakes.
@freeznet
Copy link
Member Author

freeznet commented Aug 8, 2025

@freeznet Wondering what's the disadvantages of achieving infinite retention policy by setting retentionTime and retentionSize to -1 compared to use new attribute?

@maxsxu Thanks for the comments, and I thought the retentionTime and retentionSize are limited by Kubernetes types and failed to provide -1, so I went for the additional fields as the solution. Following your comments, I have revisited the structures and updated the PR accordingly. PTAL again, thanks.

@freeznet freeznet merged commit 2aa26a3 into main Aug 13, 2025
5 checks passed
@freeznet freeznet deleted the freeznet/infinite-retention-policy branch August 13, 2025 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-info-missing This pr needs to mark a document option in description

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants