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

[WIP] Update configuration from Manager server #118

Closed
wants to merge 1 commit into from

Conversation

danbi2990
Copy link
Contributor

@danbi2990 danbi2990 commented Nov 15, 2024

Replace reload_config with update_config.

Close: #112

In this version, the `update_config` is effectively dead code (non-functional) because:
  • if a local config is provided, the updated config is ignored by policy.
  • if no local config is provided, the program cannot run normally due to the absence of last_timestamp_data.

As discussed, this will be addressed in a separate issue as follows:

  • If no local config is provided, retrieve the config from the Manager server.
  • If all attempts fail, the program will enter minimal mode.

About code details

  1. There are code patterns like writer.lock().unwrap_or_else(std::sync::PoisonError::into_inner)
    which ignore the PoisonError, assuming that the log writer should not cause a
    panic even if another thread has poisoned the lock.

  2. If log_dir changes by updating config, logs should be saved in that
    directory. This commit implements this by wrapping the file writer.
    Alternatively, tracing_subscriber::reload could be used, but it cannot
    handle cases where the directory is added or removed (as it requires
    reassigning the writer itself, which is not possible due to internal
    limitations).
    Related issue: Panic after reloading layer, combined with .with_filter tokio-rs/tracing#1629

@danbi2990 danbi2990 force-pushed the jake/impl-update-config branch 13 times, most recently from 179fb76 to 359e89a Compare November 18, 2024 05:34
@danbi2990 danbi2990 changed the title [WIP] Update configuration from Manager server Update configuration from Manager server Nov 18, 2024
@danbi2990 danbi2990 marked this pull request as ready for review November 18, 2024 05:59
src/main.rs Outdated Show resolved Hide resolved
@danbi2990 danbi2990 changed the title Update configuration from Manager server [WIP] Update configuration from Manager server Nov 20, 2024
src/main.rs Outdated Show resolved Hide resolved
@danbi2990 danbi2990 force-pushed the jake/impl-update-config branch 2 times, most recently from d4e303c to ab935b0 Compare November 21, 2024 05:01
@danbi2990 danbi2990 changed the title [WIP] Update configuration from Manager server Update configuration from Manager server Nov 21, 2024
src/logging.rs Outdated Show resolved Hide resolved
@danbi2990 danbi2990 force-pushed the jake/impl-update-config branch from ab935b0 to 29f5584 Compare November 21, 2024 05:54
src/main.rs Outdated Show resolved Hide resolved
@danbi2990 danbi2990 force-pushed the jake/impl-update-config branch from 29f5584 to 0a12183 Compare November 28, 2024 00:57
CHANGELOG.md Outdated
Comment on lines 10 to 16
### Added

- Added `update_config` request handler.

### Removed

- Removed `reload_config` request handler.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these should be combined into one item in the Changed section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied the suggested adjustment.

@danbi2990 danbi2990 force-pushed the jake/impl-update-config branch from 0a12183 to 582f786 Compare November 29, 2024 00:55
async fn reload_config(&mut self) -> Result<(), String> {
info!("start reloading configuration");
async fn update_config(&mut self) -> Result<(), String> {
info!("Updating configuration");
Copy link
Contributor

Choose a reason for hiding this comment

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

@sophie-cluml and @danbi2990,
Does calling this method guarantee that the update process will start? If not, we need to clarify that this method only attempts or plans to update the configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sophie and I discussed that the update process will be implemented in the next issue.
The next issue will proceed after the same functionality is completed for the Semi-supervised Engine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get what you said. Is this going to wait for the next issue being completed? Or, this message will be handled later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latter is right. It will be handled later because it needs to fetch the configuration from the manager server.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this PR is temporarily being halted, it would be better to mention it to let others know of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original plan was to merge this PR first and then implement the next issue.

I thought it was fine because this PR only changes the method name while maintaining the same behavior as before.

However, as you suggested, it seems reasonable to wait until the next issue is completed. For now, I will mark this PR as [WIP]. Thank you!

@danbi2990 danbi2990 force-pushed the jake/impl-update-config branch from 582f786 to 4aaa286 Compare December 2, 2024 04:05
@danbi2990 danbi2990 changed the title Update configuration from Manager server [WIP] Update configuration from Manager server Dec 4, 2024
@sophie-cluml
Copy link

sophie-cluml commented Jan 20, 2025

Since the linked issue of this PR, is handled by #131, let me close this PR. (cc. @danbi2990 When we manually close PR, we close it with a comment that describes why. This process is guided in our rule.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reload configuration upon receiving configuration-updated-notification
4 participants