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

Add ClientDeactivateThreshold in Project #454

Conversation

krapie
Copy link
Member

@krapie krapie commented Jan 27, 2023

What this PR does / why we need it:

Add ClientDeactivateThreshold in Project to set client’s deactivation threshold in individual projects for housekeeping.

This will subtitute preview DefaultHousekeepingDeactivateThreshold in global housekeeping configuration.

Current housekeeping mechanism deactivates client with provided DefaultHousekeepingDeactivateThreshold global configuration. This deactivates all clients without considering each client’s workloads.

In Yorkie, Project repesents specific workloads, and each Project has different configurations and workloads. Therefore, it will be more efficient to set deactivation conditions in Project rather then global housekeeping deactivation configuration.

For more information & context, follow this issue: #444

Which issue(s) this PR fixes:

Related #444

Special notes for your reviewer:

Below is the list to be considered before merge.

  1. Both implementation of memory and mongo of FindDeactivateCandidates() have changed to fetch all projects first, and then fetch clients by projects using get() and find(). There is limit(candidatesLimit) on clients to prevent overloading on memory, but pagination/limit on projects may also be needed. Then, what is a best pagination/limit strategy for projects on housekeeping? Any suggestions will be helpful. (Or, this may not be needed because we can shard project collections so that server can able to load all projects with no problem?)

Does this PR introduce a user-facing change?:

Users can set client's deactivation threshold in each project using Yorkie dashboard (Yorkie admin CLI)

Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

@krapie krapie changed the title Add deactivateThreshold in project (WIP) Add deactivateThreshold in project Jan 27, 2023
@krapie krapie force-pushed the feature/housekeeping-project-deactivate-threshold branch 2 times, most recently from 317535a to f600a3d Compare January 27, 2023 11:22
@krapie krapie force-pushed the feature/housekeeping-project-deactivate-threshold branch 4 times, most recently from 8a2a9c3 to 96a7ccc Compare January 27, 2023 17:49
@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #454 (fe97326) into main (3d9fe9d) will increase coverage by 0.38%.
The diff coverage is 32.14%.

@@            Coverage Diff             @@
##             main     #454      +/-   ##
==========================================
+ Coverage   46.26%   46.65%   +0.38%     
==========================================
  Files          69       69              
  Lines        5678     5749      +71     
==========================================
+ Hits         2627     2682      +55     
- Misses       2758     2763       +5     
- Partials      293      304      +11     
Impacted Files Coverage Δ
api/converter/from_pb.go 54.54% <0.00%> (-0.39%) ⬇️
api/converter/to_pb.go 63.69% <0.00%> (-0.64%) ⬇️
api/types/project.go 100.00% <ø> (ø)
server/backend/backend.go 0.00% <0.00%> (ø)
server/backend/database/project_info.go 30.00% <14.28%> (+2.72%) ⬆️
server/backend/database/mongo/client.go 8.61% <18.91%> (-0.13%) ⬇️
server/backend/config.go 55.55% <60.00%> (+1.26%) ⬆️
server/config.go 43.01% <60.00%> (-0.95%) ⬇️
server/backend/database/memory/database.go 51.76% <79.31%> (+5.11%) ⬆️
api/types/updatable_project_fields.go 100.00% <100.00%> (ø)
... and 2 more

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

@hackerwins
Copy link
Member

hackerwins commented Jan 28, 2023

Thanks for your contribution.

  1. Previous DefaultHousekeepingDeactivateThreshold in global configuration is removed. Do we need to keep this option alive or not?

It would be ok to remove it.

  1. MongoDB’s aggregation pipeline is used to avoid memory consumptions on server during FindDeactivateCandidates(), is this over engineered?

We need to check that the query(aggregation) is not putting too much load on the DB. In a production environment, the number of projects may be small, but the number of clients may be large. It would be better to process the deactivation by project and check if the index is properly set.

  1. Follow above, there are mismatch in go’s time.duration (microsecond unit) and mongoDB’s time (microsecond unit). I set time unit to microsecond, but the conversation is quite cumbersome, and this made explicit type convertion between mongoDB and go. Now ConvertBsonMillisceondToDurationMicrosecond() is needed in every projectInfo fetch. (Also ClientDeactivateThreshold in UpdatableProjectFields should be in microseconds…)
  2. UpdatableProjectFields has changed to get ClientDeactivateThreshold value for Project ClientDeactivateThreshold update. Therefore code to update project ClientDeactivateThreshold should be added, and protobuf files should be updated in Yorkie dashboard.

Because We tried to use aggregation, I think there was a restriction on the type of the field. It would be good to change the structure to run the deactivation on a per-project basis as mentioned above, and change the type of the field to the user's input format(probably a string).

  1. FindDeactivateCandidates() implementation of memory has changed to get all data get(), and iterate all data continue instead of reverseLowerBound with global offset. This should be optimized.

Yes, we need to apply proper pagination when fetching candidate clients from DB.

After this work, I think we can add the deactivation setting to the Dashboard and CLI.

@krapie krapie changed the title (WIP) Add deactivateThreshold in project (WIP) Add clientDeactivateThreshold in project Jan 28, 2023
@krapie
Copy link
Member Author

krapie commented Jan 28, 2023

Thank you for your feedbacks.

It would be better to process the deactivation by project and check if the index is properly set

Do you mean iterating project with FindDeactivateCandidatesInProject() on housekeeping? (indexing colClients with project, and performing FindDeactivateCandidatesInProject(), which will use same algorithm(query) as previous FindDeactivateCandidates())

If my understanding is correct, I think your suggestion is much better then current implementation. This will definitely reduce cumbersome code to perform deactivateThreshold on clients, and keep previous query to get deactivate candidates!

@krapie krapie force-pushed the feature/housekeeping-project-deactivate-threshold branch 12 times, most recently from d9432f2 to d5370de Compare February 11, 2023 13:17
@krapie
Copy link
Member Author

krapie commented Feb 11, 2023

I have implemented findDeactivateCandidatesPerProject() in housekeeping's FindDeactivateCandidates() which finds deactivate client candidates per project. But still there is a consideration to be resolved. See PR message for more information.

@krapie krapie marked this pull request as ready for review February 11, 2023 13:24
@krapie krapie changed the title (WIP) Add clientDeactivateThreshold in project Add clientDeactivateThreshold in project Feb 11, 2023
@krapie krapie changed the title Add clientDeactivateThreshold in project Add ClientDeactivateThreshold in Project Feb 11, 2023
@hackerwins
Copy link
Member

Both implementation of memory and mongo of FindDeactivateCandidates() have changed to fetch all projects first, and then fetch clients by projects using get() and find(). There is limit(candidatesLimit) on clients to prevent overloading on memory, but pagination/limit on projects may also be needed. Then, what is a best pagination/limit strategy for projects on housekeeping? Any suggestions will be helpful. (Or, this may not be needed because we can shard project collections so that server can able to load all projects with no problem?)

The number of projects is very small compared to the number of clients, and there are not many Yorkie users and projects yet. Therefore, it seems that there is no need to introduce pagination to the project now. How about leaving a TODO comment about the pagination of projects for now?

@hackerwins hackerwins self-requested a review February 17, 2023 04:58
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. It looks good overall.

I left a trivial comment.

server/backend/database/memory/database.go Outdated Show resolved Hide resolved
server/backend/database/memory/housekeeping_test.go Outdated Show resolved Hide resolved
server/backend/database/project_info.go Outdated Show resolved Hide resolved
@krapie krapie force-pushed the feature/housekeeping-project-deactivate-threshold branch 3 times, most recently from 858d58e to a8e4b9f Compare February 17, 2023 09:41
Add clientDeactivateThreshold in Project to set client’s deactivation threshold in individual projects for housekeeping.
This will subtitute previousw DefaultHousekeepingDeactivateThreshold in global housekeeping configuration.
@krapie krapie force-pushed the feature/housekeeping-project-deactivate-threshold branch from a8e4b9f to fe97326 Compare February 17, 2023 10:18
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

This PR will be helpful to other developers as they develop other global settings that need to be moved to project settings.

@hackerwins hackerwins merged commit c5b9a90 into yorkie-team:main Feb 17, 2023
@krapie krapie deleted the feature/housekeeping-project-deactivate-threshold branch February 17, 2023 10:50
@krapie krapie changed the title Add ClientDeactivateThreshold in Project Add ClientDeactivateThreshold in Project Mar 14, 2023
@krapie krapie changed the title Add ClientDeactivateThreshold in Project Add ClientDeactivateThreshold in Project Mar 14, 2023
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.

2 participants