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

update delete flushThreshold #18758

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

Wenbin1002
Copy link
Contributor

@Wenbin1002 Wenbin1002 commented Sep 12, 2024

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue # https://github.com/matrixorigin/MO-Cloud/issues/4050

What this PR does / why we need it:

update delete flushThreshold


PR Type

Bug fix


Description


Changes walkthrough 📝

Relevant files
Bug fix
types.go
Update `flushThreshold` value in deletion types                   

pkg/sql/colexec/deletion/types.go

  • Updated the flushThreshold value from 32 MB to 5 MB.
+1/-1     

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

Copy link

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Performance Impact
Reducing flushThreshold from 32 MB to 5 MB may impact performance. This change might lead to more frequent flushes, potentially affecting system efficiency.

Copy link

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Enhancement
Make the flush threshold configurable through environment variables

Consider making flushThreshold configurable through environment variables or a
configuration file, rather than hardcoding the value. This would allow for easier
adjustments without code changes.

pkg/sql/colexec/deletion/types.go [38-40]

 var (
-	flushThreshold = 5 * mpool.MB
+	flushThreshold = getConfigurableFlushThreshold()
 )
 
+func getConfigurableFlushThreshold() int64 {
+	if threshold := os.Getenv("FLUSH_THRESHOLD"); threshold != "" {
+		if value, err := strconv.ParseInt(threshold, 10, 64); err == nil {
+			return value * mpool.MB
+		}
+	}
+	return 5 * mpool.MB // Default value
+}
+
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: Making the flush threshold configurable enhances flexibility and adaptability, allowing changes without modifying the code, which is a significant improvement for maintainability and deployment.

8
Maintainability
Add a comment explaining the rationale for the chosen threshold value

Add a comment explaining the rationale behind choosing 5 MB as the new threshold
value. This would help future maintainers understand the decision-making process.

pkg/sql/colexec/deletion/types.go [38-40]

 var (
+	// flushThreshold is set to 5 MB to balance memory usage and performance.
+	// Lower values increase flush frequency, reducing memory usage but potentially impacting performance.
+	// Higher values decrease flush frequency, which may improve performance but increase memory usage.
 	flushThreshold = 5 * mpool.MB
 )
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: Adding a comment to explain the rationale behind the threshold value improves code readability and helps future maintainers understand the decision-making process, though it is not critical.

7

@mergify mergify bot added the kind/bug Something isn't working label Sep 12, 2024
@matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Sep 12, 2024
@mergify mergify bot merged commit dd07375 into matrixorigin:main Sep 13, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug fix kind/bug Something isn't working Review effort [1-5]: 1 size/XS Denotes a PR that changes [1, 9] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants