Skip to content

refactor: define number in core_settings#1609

Open
Nexisato wants to merge 1 commit intomainfrom
refactor/core_settings
Open

refactor: define number in core_settings#1609
Nexisato wants to merge 1 commit intomainfrom
refactor/core_settings

Conversation

@Nexisato
Copy link
Copy Markdown
Contributor

@Nexisato Nexisato commented May 9, 2026

Description

  • Define relative magic number in core_settings for [converter] env value control

@Nexisato Nexisato self-assigned this May 9, 2026
@Nexisato Nexisato added the 💪 enhancement New feature or request label May 9, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the transport and dispatch logic to use configurable settings from RunContext instead of hardcoded constants, specifically for batch intervals, record limits per request, and thread join timeouts. Corresponding unit tests were updated to support these changes using mock contexts. The review feedback recommends adding validation to the new configuration fields in CoreSettings and improving the logging behavior in the transport thread's shutdown sequence to handle potential join timeouts more accurately.

Comment on lines +94 to 95
self._thread.join(timeout=self._finish_join_timeout)
console.debug("Transport finished.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The log message "Transport finished." is misleading if the join operation times out. It is better to check if the thread is still alive and provide appropriate feedback, as a timeout might indicate that some records were not successfully uploaded before the shutdown.

Suggested change
self._thread.join(timeout=self._finish_join_timeout)
console.debug("Transport finished.")
self._thread.join(timeout=self._finish_join_timeout)
if self._thread.is_alive():
console.warning(f"Transport thread did not finish within {self._finish_join_timeout}s timeout.")
else:
console.debug("Transport finished.")

Comment on lines +22 to +36
batch_interval: float = Field(default=5.0)
"""
Batch interval (seconds) for the Transport upload thread.
Default 5.0s. Use a smaller value (e.g. 0.5) in Converter scenarios for higher throughput.
"""

max_records_per_request: int = Field(default=10_000)
"""
Maximum number of records per HTTP request in Dispatch. Default 10000.
"""

finish_join_timeout: int = Field(default=30)
"""
Timeout (seconds) when waiting for the Transport thread to exit on finish. Default 30.
"""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

It is recommended to add validation to these numeric fields to ensure they contain sensible values (e.g., positive intervals and non-negative timeouts). This prevents potential runtime errors or infinite loops if invalid values are provided via environment variables.

Suggested change
batch_interval: float = Field(default=5.0)
"""
Batch interval (seconds) for the Transport upload thread.
Default 5.0s. Use a smaller value (e.g. 0.5) in Converter scenarios for higher throughput.
"""
max_records_per_request: int = Field(default=10_000)
"""
Maximum number of records per HTTP request in Dispatch. Default 10000.
"""
finish_join_timeout: int = Field(default=30)
"""
Timeout (seconds) when waiting for the Transport thread to exit on finish. Default 30.
"""
batch_interval: float = Field(default=5.0, gt=0)
"""
Batch interval (seconds) for the Transport upload thread.
Default 5.0s. Use a smaller value (e.g. 0.5) in Converter scenarios for higher throughput.
"""
max_records_per_request: int = Field(default=10_000, gt=0)
"""
Maximum number of records per HTTP request in Dispatch. Default 10000.
"""
finish_join_timeout: int = Field(default=30, ge=0)
"""
Timeout (seconds) when waiting for the Transport thread to exit on finish. Default 30.
"""

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

Labels

💪 enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant