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] Add delete_sampling_policy api #353

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

BLYKIM
Copy link
Contributor

@BLYKIM BLYKIM commented Apr 13, 2023

  • Add a GraphQL API that can delete timeseries from the database using an ID.

Issue: #351

@BLYKIM BLYKIM force-pushed the bly/delete_timeseries branch 2 times, most recently from 8b46f06 to b09acc7 Compare April 17, 2023 04:31
@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2023

Codecov Report

Patch coverage: 93.33% and project coverage change: +0.24 🎉

Comparison is base (3984e48) 68.15% compared to head (ea7986d) 68.39%.

❗ Current head ea7986d differs from pull request most recent head 9dc452d. Consider uploading reports for the commit 9dc452d to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
+ Coverage   68.15%   68.39%   +0.24%     
==========================================
  Files          19       19              
  Lines        2779     2791      +12     
==========================================
+ Hits         1894     1909      +15     
+ Misses        885      882       -3     
Impacted Files Coverage Δ
src/graphql.rs 71.27% <ø> (ø)
src/publish.rs 69.06% <66.66%> (ø)
src/graphql/timeseries.rs 91.66% <100.00%> (+4.16%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@BLYKIM BLYKIM force-pushed the bly/delete_timeseries branch 4 times, most recently from 135f9d5 to ea7986d Compare April 19, 2023 05:16
@BLYKIM BLYKIM requested review from sehkone and syncpark April 19, 2023 05:18
@BLYKIM BLYKIM changed the title [WIP] Add delete_sampling_policy api Add delete_sampling_policy api Apr 19, 2023
@BLYKIM BLYKIM force-pushed the bly/delete_timeseries branch 2 times, most recently from 9dc452d to 208ebac Compare April 19, 2023 05:38
@msk
Copy link
Contributor

msk commented Apr 19, 2023

giganto에서 모든 데이터는 오래되면 삭제하도록 되어 있으니 그냥 놔 두면 나중에 삭제될텐데 굳이 미리 삭제할 필요가 있나요? 데이터를 삭제하려면 이 PR에서처럼 바로 삭제하면 안 되고, 현재 그 데이터를 가져가고 있는 클라이언트(REconverge)가 있는지 확인해서 그 연결이 종료된 이후에 하도록 해야 분석이 잘못되는 걸 방지할 수 있을텐데요. 데이터 삭제가 필요한 상황과 관련된 예시를 제공해 주실 수 있나요?

@syncpark
Copy link
Contributor

syncpark commented Apr 20, 2023

giganto에서 모든 데이터는 오래되면 삭제하도록 되어 있으니 그냥 놔 두면 나중에 삭제될텐데 굳이 미리 삭제할 필요가 있나요? 데이터를 삭제하려면 이 PR에서처럼 바로 삭제하면 안 되고, 현재 그 데이터를 가져가고 있는 클라이언트(REconverge)가 있는지 확인해서 그 연결이 종료된 이후에 하도록 해야 분석이 잘못되는 걸 방지할 수 있을텐데요. 데이터 삭제가 필요한 상황과 관련된 예시를 제공해 주실 수 있나요?

이번 삭제 요청은 Web UI에서 관리자가 정책을 삭제하려고 클릭했을 때 Crusher와 Giganto에게 먼저 정책과 데이터 삭제를 요청하고, 성공할 경우 REview가 관리하는 정책도 삭제하려는 시나리오입니다.

  1. 관리자가 Web UI에서 정책의 삭제를 요청
  2. Web UI가 Giganto, Crusher에게 정책과 관련된 데이터와 운영중인 정책의 삭제를 요청
  3. 1, 2가 성공하면 REview의 Policy를 삭제

말씀하신 대로 REconverge가 해당 Policy를 참조할 수 있으므로 1,2번 사이에 해당 Policy를 참조하는 모델이 있는지, 운영중인지 확인하고, 있다면 해당 모델을 먼저 정지시키는 단계를 추가할 필요가 있습니다.

이와 관련된 이슈는 Aice-web에 등록되어있습니다. 위 내용을 이슈에 반영해서 수정하겠습니다.

Copy link
Contributor

@syncpark syncpark left a comment

Choose a reason for hiding this comment

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

삭제 관련 논의가 끝난 후에 이후 작업을 진행합시다.

@BLYKIM BLYKIM changed the title Add delete_sampling_policy api [WIP] Add delete_sampling_policy api Apr 21, 2023
- Add a GraphQL API that can delete timeseries from the database using an ID.

Issue: #351
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.

4 participants