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

fix: judge file server changed #1970

Merged
merged 3 commits into from
Dec 18, 2024
Merged

fix: judge file server changed #1970

merged 3 commits into from
Dec 18, 2024

Conversation

Abingcbc
Copy link
Collaborator

@Abingcbc Abingcbc commented Dec 17, 2024

问题

在多个配置加载时,需要根据input_file判断isFileServerInputChanged,从而决定是否启动FileServer。但是之前PR修改,导致后一个配置的结果可以覆盖前一个配置。

修复

任意一个配置改变了input_file,isFileServerInputChanged都应该是true

bool PipelineManager::CheckIfFileServerUpdated(const Json::Value& config) {
string inputType = config["Type"].asString();
return inputType == "input_file" || inputType == "input_container_stdio";
bool PipelineManager::CheckIfFileServerUpdated(PipelineConfigDiff& diff) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

既然已经出现了之前测试不到的情况,是不是应该针对性的补充用例?

}
}
for (const auto& config : diff.mModified) {
string inputType = (*config.mInputs[0])["Type"].asString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

load配置的地方是否能够保证[0]的合法性?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

前面配置解析会检验input至少包含一个,保证这里加载流水线一定是合法的

@Abingcbc Abingcbc merged commit c5be143 into main Dec 18, 2024
17 checks passed
@henryzhx8 henryzhx8 added the bug Something isn't working label Dec 19, 2024
@henryzhx8 henryzhx8 added this to the v3.0 milestone Dec 19, 2024
@henryzhx8 henryzhx8 deleted the fix-pipeline branch December 28, 2024 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants