Skip to content

performance improvement of getting devfile registries#2495

Merged
dgolovin merged 12 commits intoredhat-developer:mainfrom
msivasubramaniaan:reduce-devfile-registry-odo-call
Aug 17, 2022
Merged

performance improvement of getting devfile registries#2495
dgolovin merged 12 commits intoredhat-developer:mainfrom
msivasubramaniaan:reduce-devfile-registry-odo-call

Conversation

@msivasubramaniaan
Copy link
Copy Markdown
Collaborator

@msivasubramaniaan msivasubramaniaan commented Jul 28, 2022

Signed-off-by: msivasubramaniaan msivasub@redhat.com

Fix #2496

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
@msivasubramaniaan msivasubramaniaan self-assigned this Jul 28, 2022
…pdate on the devfile registries

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
Copy link
Copy Markdown
Collaborator

@dgolovin dgolovin left a comment

Choose a reason for hiding this comment

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

Please set indentation to 4 spaces.

Comment thread src/extension.ts Outdated
Comment thread src/extension.ts Outdated
Comment thread src/registriesView.ts Outdated
Comment thread src/registriesView.ts Outdated
Comment thread src/registriesView.ts Outdated
Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
@msivasubramaniaan
Copy link
Copy Markdown
Collaborator Author

Please set indentation to 4 spaces.

yes I have the same settings only

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
Copy link
Copy Markdown
Collaborator

@dgolovin dgolovin left a comment

Choose a reason for hiding this comment

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

Better, but still need some work in several places.

  1. I agree we can keep component descriptors array in ComponentTypesView class instance.
  2. Your implementation works, but it is not easy to understand and I don't think it is good idea to use subscriber to send events from member function to the same class instance when you can just use this.

You have ComponentTypesView instance and RegistryViewLoader instance. I would suggest to get rid of rxjs and use EventEmitter to nodify ComponentTypesView clients (RegistryViewer in this case) about component description update and the need to refresh.

@msivasubramaniaan
Copy link
Copy Markdown
Collaborator Author

Better, but still need some work in several places.

  1. I agree we can keep component descriptors array in ComponentTypesView class instance.
  2. Your implementation works, but it is not easy to understand and I don't think it is good idea to use subscriber to send events from member function to the same class instance when you can just use this.

You have ComponentTypesView instance and RegistryViewLoader instance. I would suggest to get rid of rxjs and use EventEmitter to nodify ComponentTypesView clients (RegistryViewer in this case) about component description update and the need to refresh.

As discussed using subject for only sending notification on refresh the screen.

@mohitsuman
Copy link
Copy Markdown
Contributor

LGTM wrt to the UI part of it with loader and icon. Once @dgolovin is fine with the code workflow, we can merge.

mohitsuman
mohitsuman previously approved these changes Aug 4, 2022
Comment thread src/registriesView.ts Outdated
Comment thread src/registriesView.ts Outdated
Comment thread src/registriesView.ts Outdated
Comment thread src/registriesView.ts Outdated
Comment thread src/extension.ts Outdated
Copy link
Copy Markdown
Collaborator

@dgolovin dgolovin left a comment

Choose a reason for hiding this comment

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

One last change is needed.

Comment thread src/registriesView.ts
Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 17, 2022

Codecov Report

Merging #2495 (d608d58) into main (30bf2bb) will increase coverage by 0.57%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #2495      +/-   ##
==========================================
+ Coverage   66.58%   67.15%   +0.57%     
==========================================
  Files          55       55              
  Lines        3585     3626      +41     
  Branches      649      659      +10     
==========================================
+ Hits         2387     2435      +48     
+ Misses       1198     1191       -7     
Impacted Files Coverage Δ
src/odo.ts 69.67% <ø> (+1.21%) ⬆️
src/registriesView.ts 31.75% <40.00%> (+3.30%) ⬆️
src/webview/devfile-registry/registryViewLoader.ts 27.63% <66.66%> (+8.80%) ⬆️
src/util/kubeUtils.ts 87.71% <90.00%> (+2.00%) ⬆️
src/extension.ts 83.58% <100.00%> (+0.24%) ⬆️

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

@dgolovin dgolovin merged commit 3640689 into redhat-developer:main Aug 17, 2022
@msivasubramaniaan msivasubramaniaan deleted the reduce-devfile-registry-odo-call branch August 17, 2022 06:44
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.

Improve loading time for registry viewer

3 participants