-
Notifications
You must be signed in to change notification settings - Fork 20
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
Hkube gc #1287
Hkube gc #1287
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 86 of 96 files at r1, 2 of 2 files at r2, 3 of 3 files at r3, 9 of 34 files at r4.
Reviewable status: 73 of 98 files reviewed, 7 unresolved discussions (waiting on @nassiharel and @yehiyam)
core/hkube-gc/bootstrap.js, line 30 at r3 (raw file):
await cleanerManager.init(config); await Promise.all(modules.map(m => m.init(config))); // await healthchecks.initAndStart(main.healthchecks, () => runner.checkHealth(main.healthchecks.maxDiff), main.serviceName);
need to think how to add the healthchecks back
at least check that each of the cronjobs that started also finished in a reasonable time
core/hkube-gc/lib/cleaners/storage/indices-cleaner.js, line 8 at r3 (raw file):
class IndicesCleaner extends BaseCleaner { async getJobsToDelete({ indices, maxAgeResults }) { return super.getJobsToDelete({ indices, maxAge: maxAgeResults });
why maxAgeResults
in IndicesCleaner
and not ResultsCleaner
?
core/hkube-gc/lib/cleaners/storage/promise-wrapper.js, line 1 at r3 (raw file):
const promiseWrapper = (func) => {
why not use Promise.allSettled
instead?
core/hkube-gc/lib/core/kind-cleaner.js, line 4 at r4 (raw file):
const storeManager = require('../helpers/store-manager'); class Cleaner {
Isn't it better to give each class its own name?
core/hkube-gc/lib/cleaner-manager.js, line 14 at r3 (raw file):
cleaners.forEach(name => { const config = options.cleanerSettings[name]; if (config.enabled) {
Write to log if not enabled, so we can know why it didn't run
core/hkube-gc/lib/cleaner-manager.js, line 17 at r3 (raw file):
const Cleaner = require(`../${CLEANERS_PATH}/${name}`); // eslint-disable-line const cleaner = new Cleaner({ config, name }); cleaner.init({ cleanMethod: (...args) => cleaner.clean(...args) });
Why pass the cleanMethod, and not call the overloaded method in each cleaner
core/hkube-gc/lib/core/cleaner-manager.js, line 30 at r4 (raw file):
} _cronFormat(cron) {
put this in base-cleaner
and call it after the init
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 63 of 99 files reviewed, 7 unresolved discussions (waiting on @yehiyam)
core/hkube-gc/bootstrap.js, line 30 at r3 (raw file):
Previously, yehiyam wrote…
need to think how to add the healthchecks back
at least check that each of the cronjobs that started also finished in a reasonable time
Done.
core/hkube-gc/lib/cleaners/storage/indices-cleaner.js, line 8 at r3 (raw file):
Previously, yehiyam wrote…
why
maxAgeResults
inIndicesCleaner
and notResultsCleaner
?
This function is redundant.
core/hkube-gc/lib/cleaners/storage/promise-wrapper.js, line 1 at r3 (raw file):
Previously, yehiyam wrote…
why not use
Promise.allSettled
instead?
You are right, This is an old copy-paste
core/hkube-gc/lib/core/kind-cleaner.js, line 4 at r4 (raw file):
Previously, yehiyam wrote…
Isn't it better to give each class its own name?
Done.
core/hkube-gc/lib/cleaner-manager.js, line 14 at r3 (raw file):
Previously, yehiyam wrote…
Write to log if not enabled, so we can know why it didn't run
Done.
core/hkube-gc/lib/cleaner-manager.js, line 17 at r3 (raw file):
Previously, yehiyam wrote…
Why pass the cleanMethod, and not call the overloaded method in each cleaner
Done.
core/hkube-gc/lib/core/cleaner-manager.js, line 30 at r4 (raw file):
Previously, yehiyam wrote…
put this in
base-cleaner
and call it after the init
Done.
/deploy |
/deploy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 34 files at r4, 13 of 27 files at r5, 99 of 99 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nassiharel)
.github/labeler.yml, line 30 at r5 (raw file):
gc-service
You need to rename the folder also
core/hkube-gc/config/main/config.base.js, line 18 at r5 (raw file):
prefix: 'api', poweredBy: 'HKube GC', bodySizeLimit: process.env.BODY_SIZE_LIMIT || '2000mb'
2GB? isn't it excessive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 102 of 103 files reviewed, 2 unresolved discussions (waiting on @yehiyam)
.github/labeler.yml, line 30 at r5 (raw file):
Previously, yehiyam wrote…
gc-service
You need to rename the folder also
Where? I already changed it
core/hkube-gc/config/main/config.base.js, line 18 at r5 (raw file):
Previously, yehiyam wrote…
2GB? isn't it excessive?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nassiharel)
* add hkube-gc * fix lint * fix action name * fix require path * fix logger config * fix logger config * fix PR * fix PR * fix PR * fix PR * fix PR * fix PR * feat: install missing devDependencies * feat: install missing devDependencies * feat: PR Issues * fix: reduce bodySizeLimit .... bump version [skip ci]
* add hkube-gc * fix lint * fix action name * fix require path * fix logger config * fix logger config * fix PR * fix PR * fix PR * fix PR * fix PR * fix PR * feat: install missing devDependencies * feat: install missing devDependencies * feat: PR Issues * fix: reduce bodySizeLimit .... bump version [skip ci]
This change is