Skip to content

Conversation

@DaanHoogland
Copy link
Contributor

Description

This PR resolves conflicts in #9260

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI
  • test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

@codecov
Copy link

codecov bot commented Aug 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 17.35%. Comparing base (3c4f458) to head (63d68a3).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main   #11540   +/-   ##
=========================================
  Coverage     17.35%   17.35%           
- Complexity    15232    15236    +4     
=========================================
  Files          5888     5888           
  Lines        525733   525733           
  Branches      64164    64164           
=========================================
+ Hits          91255    91266   +11     
+ Misses       424178   424168   -10     
+ Partials      10300    10299    -1     
Flag Coverage Δ
uitests 3.63% <ø> (ø)
unittests 18.39% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sonarqubecloud
Copy link

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 makes the Jetty server thread pool configuration configurable through the server.properties file instead of being hardcoded. Previously, the minimum and maximum thread counts were fixed at 10 and 500 respectively.

Key changes:

  • Added configurable thread pool properties with defaults matching the previous hardcoded values
  • Replaced hardcoded thread pool configuration with property-based configuration
  • Added documentation for the new configuration options

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
ServerDaemon.java Added thread pool configuration properties and replaced hardcoded values with configurable ones
server.properties.in Added commented configuration examples for the new thread pool settings

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +154 to +155
setMinThreads(Integer.valueOf(properties.getProperty(THREADS_MIN, "10")));
setMaxThreads(Integer.valueOf(properties.getProperty(THREADS_MAX, "500")));
Copy link

Copilot AI Sep 1, 2025

Choose a reason for hiding this comment

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

Missing input validation for thread configuration values. The code should validate that minThreads > 0, maxThreads > 0, and minThreads <= maxThreads to prevent invalid thread pool configurations that could cause runtime issues.

Suggested change
setMinThreads(Integer.valueOf(properties.getProperty(THREADS_MIN, "10")));
setMaxThreads(Integer.valueOf(properties.getProperty(THREADS_MAX, "500")));
setMaxThreads(Integer.valueOf(properties.getProperty(THREADS_MAX, "500")));
// Validate thread configuration
if (minThreads <= 0) {
logger.warn("Invalid minThreads value (" + minThreads + "). Setting to default (10).");
setMinThreads(10);
}
if (maxThreads <= 0) {
logger.warn("Invalid maxThreads value (" + maxThreads + "). Setting to default (500).");
setMaxThreads(500);
}
if (minThreads > maxThreads) {
logger.warn("minThreads (" + minThreads + ") is greater than maxThreads (" + maxThreads + "). Setting minThreads to maxThreads.");
setMinThreads(maxThreads);
}

Copilot uses AI. Check for mistakes.
@DaanHoogland DaanHoogland marked this pull request as ready for review September 1, 2025 14:36
Copy link
Contributor

@JoaoJandre JoaoJandre left a comment

Choose a reason for hiding this comment

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

CLGTM

@DaanHoogland
Copy link
Contributor Author

I am going to assume testing is still valid from the original ticket (leaving on the edge) cc @rosi-shapeblue

@DaanHoogland DaanHoogland merged commit 76ef8d3 into main Sep 2, 2025
49 checks passed
@DaanHoogland DaanHoogland deleted the pr/9260 branch September 2, 2025 06:56
@weizhouapache weizhouapache added this to the 4.22.0 milestone Sep 2, 2025
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Sep 4, 2025
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.

4 participants