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: avoid db query on sync mode all #527

Merged
merged 1 commit into from
Jun 20, 2023
Merged

fix: avoid db query on sync mode all #527

merged 1 commit into from
Jun 20, 2023

Conversation

fengmk2
Copy link
Member

@fengmk2 fengmk2 commented Jun 20, 2023

@fengmk2 fengmk2 added the bug Something isn't working label Jun 20, 2023
ctx.redirect(downloadUrl);
return;
}

// check package version in database
const pkg = await this.getPackageEntityByFullname(fullname);
const packageVersion = await this.getPackageVersionEntity(pkg, version);

Choose a reason for hiding this comment

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

此代码 patch 主要是增加了一个条件判断和对对应业务的处理:

  1. 新增 import 语句导入 SyncMode 常量。
  2. 在 download 方法中新增对变量 this.config.cnpmcore.syncMode 的判断,如果该值等于 SyncMode.all 且下载链接存在,会先使用该下载链接以避免数据库查询,然后增加包版本计数器(这里的具体实现需要结合其他代码进行评估),最后进行重定向。
  3. 如果不存在下载链接,则继续检查数据库中是否存在对应的包和版本记录。根据语境,该方法在用户请求下载时会调用。

评估该 patch 后可能含有的缺陷和改进建议:

  • 需要确认 SyncMode.all 判断条件的意义和设计合理性
  • 涉及到 packageManagerService.plusPackageVersionCounter() 功能的实现需要确认其正确性和是否遗漏其他关键操作(例如异常处理、事务等)
  • 需要补充注释来解释相关意思和解释该 patch 是出于何种原因而做出的

Copy link
Member

@elrrrrrrr elrrrrrrr left a comment

Choose a reason for hiding this comment

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

+1

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #527 (6f23ae5) into master (eb91b83) will decrease coverage by 0.03%.
The diff coverage is 37.50%.

@@            Coverage Diff             @@
##           master     #527      +/-   ##
==========================================
- Coverage   97.00%   96.97%   -0.03%     
==========================================
  Files         174      174              
  Lines       16538    16545       +7     
  Branches     2162     2163       +1     
==========================================
+ Hits        16043    16045       +2     
- Misses        495      500       +5     
Impacted Files Coverage Δ
...rt/controller/package/DownloadPackageVersionTar.ts 90.00% <37.50%> (-5.90%) ⬇️

@fengmk2 fengmk2 merged commit 49855d9 into master Jun 20, 2023
11 of 13 checks passed
@fengmk2 fengmk2 deleted the avoid-db-query branch June 20, 2023 02:15
fengmk2 pushed a commit that referenced this pull request Jun 20, 2023
[skip ci]

## [3.34.4](v3.34.3...v3.34.4) (2023-06-20)

### Bug Fixes

* avoid db query on sync mode all ([#527](#527)) ([49855d9](49855d9)), closes [/github.com//pull/522/files#r1234655574](https://github.com/cnpm//github.com/cnpm/cnpmcore/pull/522/files/issues/r1234655574)
@fengmk2
Copy link
Member Author

fengmk2 commented Jun 20, 2023

恢复了,每次都查询 db 支撑不住,并发请求 tcp 连接数达到峰值
image

hezhengxu2018 pushed a commit that referenced this pull request Jun 20, 2023
[skip ci]

## [3.34.4](v3.34.3...v3.34.4) (2023-06-20)

### Bug Fixes

* avoid db query on sync mode all ([#527](#527)) ([49855d9](49855d9)), closes [/github.com//pull/522/files#r1234655574](https://github.com/cnpm//github.com/cnpm/cnpmcore/pull/522/files/issues/r1234655574)
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.

2 participants