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.

Sign up for GitHub

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

Jump to bottom

Add sap hana scale up cost optimized and performance optimized discovery #3152

Merged
merged 15 commits into from
Nov 28, 2024
Merged

Add sap hana scale up cost optimized and performance optimized discovery #3152

merged 15 commits into from
Nov 28, 2024

Conversation

EMaksy
Copy link
Member

@EMaksy EMaksy commented Nov 15, 2024 •
edited
Loading

Description

This pr now adds a scenario discovery, with this, as a user I can see which HANA scale-up scenario I'm running on a cluster.

Main features.

  • Multiple SID's are displayed in ClusterList, HostList, and ClusterDetails view. Links are redirecting to the correct sap database.
  • Scenario discovery between Cost Opt. or Perf. Opt.
  • Updated factory
  • Additional sids in the overview are sorted by alphabet

Note: Scale out and ASCS/ERS untouched.

Preview

ClusterList view:
image

HostList view:
image

ClusterDetails View Cost Opt:
image

ClusterDetails View Perf. Opt.
image

How was this tested?

  • Fixed existing tests
  • Added frontend tests
  • Added backend test for scale up scenario with payload
  • Fixed brocken storybook stories and added new scenarios

@EMaksy EMaksy added javascript Pull requests that update Javascript code elixir Pull requests that update Elixir code test env Create an ephimeral environment for the pr branch labels Nov 15, 2024
@EMaksy EMaksy self-assigned this Nov 15, 2024
arbulu89
arbulu89 requested changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Looking good overall.
Some comments.
I see that tests and storybook updates are missing

@EMaksy EMaksy force-pushed the hana_cost_optimized branch 3 times, most recently from 120cb10 to 41d8bce Compare November 27, 2024 09:21
@EMaksy EMaksy force-pushed the hana_cost_optimized branch from 675fc2b to c2ea5e4 Compare November 27, 2024 14:14
@EMaksy EMaksy force-pushed the hana_cost_optimized branch from 8d51e5b to c808f5f Compare November 27, 2024 15:14
@EMaksy EMaksy marked this pull request as ready for review November 27, 2024 15:28
@EMaksy EMaksy requested a review from arbulu89 November 27, 2024 15:28
arbulu89
arbulu89 requested changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

hey @EMaksy
The backend is all good (maybe I wrote some small nitpick).
I have written comments about the frontend. Small details, but I would like to fix them

@EMaksy EMaksy force-pushed the hana_cost_optimized branch from 90016ad to d54a8e6 Compare November 28, 2024 09:52
@EMaksy EMaksy force-pushed the hana_cost_optimized branch from d54a8e6 to dca3a9d Compare November 28, 2024 09:57
arbulu89
arbulu89 approved these changes Nov 28, 2024
View reviewed changes
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Green light!

nelsonkopliku
nelsonkopliku approved these changes Nov 28, 2024
View reviewed changes
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Nothing to add.

Just for me to better understand:

  • perf opt and cost opt are only related to hana scale up (not scale out)
  • definition rule is: no additional sids -> perf opt, some additional sids -> cost opt

Am I getting it right?

@arbulu89
Copy link
Contributor

Nothing to add.

Just for me to better understand:

* perf opt and cost opt are only related to hana scale up (not scale out)

* definition rule is: no additional sids -> perf opt, some additional sids -> cost opt

Am I getting it right?

Yes, you are right!

dottorblaster
dottorblaster approved these changes Nov 28, 2024
View reviewed changes
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor thing that I happened to see 👍

Very good job!

assets/js/lib/model/clusters.js
};

export const getClusterScenarioLabel = (type) =>
clusterScenarioLabels[type] || '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the ||? Doesn't it work the same with just the access to the object?

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept the default ''value for Hana Scale out case otherwise
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough 😄
For the future remember that we can also use get from lodash for better default semantics 👍

Thank you!

@EMaksy EMaksy merged commit cccfcee into main Nov 28, 2024
30 checks passed
@EMaksy EMaksy deleted the hana_cost_optimized branch November 28, 2024 11:15
@EMaksy EMaksy mentioned this pull request Dec 6, 2024
Merged
@EMaksy EMaksy mentioned this pull request Dec 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dottorblaster dottorblaster dottorblaster approved these changes

@nelsonkopliku nelsonkopliku nelsonkopliku approved these changes

@arbulu89 arbulu89 arbulu89 approved these changes

Assignees

@EMaksy EMaksy

Labels
elixir Pull requests that update Elixir code env Create an ephimeral environment for the pr branch javascript Pull requests that update Javascript code test
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@EMaksy @arbulu89 @dottorblaster @nelsonkopliku