-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Code] add NodeRepositoriesService to watch new repositories on local node #44677
Conversation
Pinging @elastic/code |
💚 Build Succeeded |
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.
mostly good.
localRemovedRepo.metadata | ||
)}` | ||
); | ||
this.localRepos.delete(localRemovedRepo.metadata.uri); |
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.
do we need to some additional steps to clean up the repository in the filesystem?
Also, since we won't have the repo size load auto-balancing in the first version or in the near future, I am just wondering what would be the case when a repo can be removed from a node because of cluster state change? A node failes and then comes back to live again?
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.
Sure, this is a problem, and I would like to leave it to future PRs, because that involves gathering and maintaining the states of local repositories and need more work and discussion.
By the way, once we have the implementation to clean up the repository based on cluster state change, it could also be used to delete the repository from the cluster: we can just mark the repository as 'deleting' in ES, the node where the repository resides will see the change and found the repository doesn't belongs to it any more and delete the repo from it. And if the clean up is triggered by the action of 'deleting', then the node can delete related indices from ES(or we can do this at the very beginning?) and mark the repository as 'deleted'(or whatever we do now).
If we do it like this, I think the cluster mode is evolving towards a cluster state driven implementation.
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.
maybe open an issue tracking this and implement later when kibana HA is ready
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.
@fantapsody it may make sense to add a TODO comment here.
currentState: RepositoryState; | ||
} | ||
|
||
enum RepositoryState { |
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.
there is a similar enum here:
export enum RepoState { |
we may consider move it to the code/common
or code/model
folder in case of duplications.
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.
cool, I will change to use RepoState
instead.
1. catch exceptions from cluster state listener 2. poll cluster state change immediately after importing a new repo 3. use existings RepoState
2462373
to
578d270
Compare
💚 Build Succeeded |
1. add TODO for repo clean ups 2. do not start reclone scheduler in cluster mode
💚 Build Succeeded |
x-pack/legacy/plugins/code/server/distributed/cluster/node_repositories_service.ts
Show resolved
Hide resolved
… node (elastic#44677) * add NodeRepositoriesService to watch new repositories on local node. * catch exceptions from cluster state listener * add TODO for repo clean ups * do not start reclone scheduler in cluster mode
…ete-for-distance_feature * 'master' of github.com:elastic/kibana: (89 commits) Replace TSVB timeseries charts with elastic-charts (elastic#33558) [TSVB][Top N aggregation] Unable to deal with negative values (elastic#43581) [alerting] Adds Action Type configuration support and whitelisting (elastic#44483) FTR: fix WebDriver Actions calls (elastic#44605) [Code] add NodeRepositoriesService to watch new repositories on local node (elastic#44677) [skip-ci][Maps] Improve Maps intro page (elastic#44721) [Maps] Update titles and descriptions for data sources (elastic#44833) Types + Extract Integration Util (elastic#44433) Downgrade log level from info to debug for cases when we cannot handle authentication attempt. (elastic#44933) [Reporting] Remove Chome stdout/stderr observables, Add Browser Logger observable (elastic#44359) Update Jest script to output coverage (elastic#44447) [ftr] support --kibana-install-dir flag (elastic#44552) [WATCHER] Allow user to set a threshold value of 0 (elastic#44810) Remove injectI18n in dashboard plugin. (elastic#44580) [Graph] Save modal (elastic#44261) Use external script for the OIDC Implicit flow handler page. (elastic#44866) disable router prefixing with pluginId (elastic#44855) [SIEM] Fix bug on url + inspect functionality on hosts/hostDetails page (elastic#44671) [ML] File data viz limiting uploaded doc chunk size (elastic#44768) [code] Append go env variable 'GOCACHE' to go lsp spawn command. (elastic#44864) ...
…plate * 'master' of github.com:elastic/kibana: (91 commits) [APM] Make number of x ticks responsive to the plot width (elastic#44870) [ML] Single metric viewer: Fix top nav refresh behaviour. (elastic#44860) Replace TSVB timeseries charts with elastic-charts (elastic#33558) [TSVB][Top N aggregation] Unable to deal with negative values (elastic#43581) [alerting] Adds Action Type configuration support and whitelisting (elastic#44483) FTR: fix WebDriver Actions calls (elastic#44605) [Code] add NodeRepositoriesService to watch new repositories on local node (elastic#44677) [skip-ci][Maps] Improve Maps intro page (elastic#44721) [Maps] Update titles and descriptions for data sources (elastic#44833) Types + Extract Integration Util (elastic#44433) Downgrade log level from info to debug for cases when we cannot handle authentication attempt. (elastic#44933) [Reporting] Remove Chome stdout/stderr observables, Add Browser Logger observable (elastic#44359) Update Jest script to output coverage (elastic#44447) [ftr] support --kibana-install-dir flag (elastic#44552) [WATCHER] Allow user to set a threshold value of 0 (elastic#44810) Remove injectI18n in dashboard plugin. (elastic#44580) [Graph] Save modal (elastic#44261) Use external script for the OIDC Implicit flow handler page. (elastic#44866) disable router prefixing with pluginId (elastic#44855) [SIEM] Fix bug on url + inspect functionality on hosts/hostDetails page (elastic#44671) ...
… node (elastic#44677) * add NodeRepositoriesService to watch new repositories on local node. * catch exceptions from cluster state listener * add TODO for repo clean ups * do not start reclone scheduler in cluster mode
Related issue: https://github.com/elastic/code/issues/1548
Summary
Summarize your PR. If it involves visual changes include a screenshot or gif.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] Unit or functional tests were updated or added to match the most common scenarios[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
[ ] This was checked for breaking API changes and was labeled appropriately[ ] This includes a feature addition or change that requires a release note and was labeled appropriately