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

[MD] Able to Hide "Local Cluster" option from datasource DropDown and Index Pattern creation page #5754

Closed
Tracked by #5752
xinruiba opened this issue Jan 29, 2024 · 15 comments
Assignees
Labels
enhancement New feature or request v2.13.0

Comments

@xinruiba
Copy link
Member

xinruiba commented Jan 29, 2024

Is your feature request related to a problem? Please describe.

When multiple data source feature is enabled, some user will not need "Local Cluster" option shows up under dataSource DropDown and Index Pattern creation page anymore.

This is the feature requirement to enabled / disable "local cluster" option from datasource dropdown and index pattern creation page when MD enabled.

With this feature created successfully, follow changes should take into place:

Add Sample Data: Local Cluster option can be enabled/disabled based on yml config
image

Screenshot 2024-02-08 at 5 32 16 PM

Dev Tools: Local Cluster option can be enabled/disabled based on yml config
image (2)

Screenshot 2024-02-08 at 5 32 28 PM

Index Pattern: Use Default DataSource can be enabled/disabled based on yml config
image (3)

Screenshot 2024-02-08 at 5 32 42 PM

POC demo

DefaultCluster.mov

Describe the solution you'd like

  1. Implement above changes in datasource picker and Index Pattern selection page
  2. Put those change under a configuration in yml configuration file [Proposal] Customize local cluster support in multi data source plugin #5696

Describe alternatives you've considered

  1. For implementations, don't think there are any alternatives to enabled/disabled local cluster without providing this feature.
  2. For configuration, we can use this but it not apply to correct feature level since user might still want to use local open-search cluster just for meta data storage, but not for querying.

Additional context
#5696
In order to implement the feature, we need:

  1. Configuration to enable / disable default cluster under multiple datasource plugin [Proposal] Customize local cluster support in multi data source plugin #5696
  2. In dev-tool and sample data page, we should use centralized datasource picker UX component to render datasource options to avoid duplicate UX implementation Add datasource picker component and use it in devtools and tutorial page when multiple datasource is enabled #5756
  3. In dev-tool and sample data page, we should able to enable / disable "Local Cluster" option from datasource picker [MD] Able to Hide "Local Cluster" option from datasource DropDown and Index Pattern creation page #5754
  4. In index-pattern page, we should remove "Use default data source" option [MD] Able to Hide "Local Cluster" option from datasource DropDown and Index Pattern creation page #5754
  5. In server side, we should provide error handling when request ID is empty and "Local Cluster" not enabled. Datasource id is required if multiple datasource is enabled and no default cluster supported #5751
@bandinib-amzn
Copy link
Member

bandinib-amzn commented Jan 30, 2024

Thanks for creating issue. Can you add more details about what will be the the UI changes or user experience any for different combination of settings?

e.g.

Configuration Result
dataSource.Enabled = True + data_source.defaultCluster = True What should user expect?
dataSource.Enabled = False + data_source.defaultCluster = True What should user expect?
dataSource.Enabled = True + data_source.defaultCluster = False What should user expect?
dataSource.Enabled = False + data_source.defaultCluster = False What should user expect?
Not configured both the settings What should user expect?

Can you add user story with more details why you are adding this feature flag?

What feature you are bringing with this feature flag?

Also is this feature flag impacts UI? if yes, how are we planning to pass this feature flag value to browser side.

@bandinib-amzn
Copy link
Member

This #5717 might change your approach to fix the issue.

@bandinib-amzn
Copy link
Member

I would change the main heading of the issue to describe more about what feature your developing. Adding feature flag is one of the step developing that feature. You can create another issues for subtasks created for achieving this feature and track them here.

@xinruiba xinruiba changed the title Add a new feature flag under data_source to enable/disabled default cluster [MD] Enable "Local Cluster" option removing from datasource DropDown Feb 2, 2024
@xinruiba xinruiba changed the title [MD] Enable "Local Cluster" option removing from datasource DropDown [MD] Removing "Local Cluster" option from datasource DropDown Feb 2, 2024
@xinruiba xinruiba changed the title [MD] Removing "Local Cluster" option from datasource DropDown [MD] Removing "Local Cluster" option from datasource DropDown and Index Pattern creation page Feb 2, 2024
@xinruiba
Copy link
Member Author

xinruiba commented Feb 2, 2024

I would change the main heading of the issue to describe more about what feature your developing. Adding feature flag is one of the step developing that feature. You can create another issues for subtasks created for achieving this feature and track them here.

@bandinib-amzn thanks for all the comments, I've updated the description of this feature with task break down and more details.

Please feel free to add more comments if there are any other concerns or suggestions. Thanks~

@xinruiba
Copy link
Member Author

xinruiba commented Feb 2, 2024

Happy to work on it

@bandinib-amzn
Copy link
Member

bandinib-amzn commented Feb 2, 2024

When multiple data source feature is enabled, users will be in confusion with "Local Cluster" option still shows up under dataSource DropDown and Index Pattern creation page.

This is not correct. User can have both multi data source enabled and local cluster running.

For feature flag, we can use this but it not apply to correct feature level.

Can you please elaborate more why we can't use this and what benefits you are bringing in by introducing new feature flag?

@bandinib-amzn
Copy link
Member

If you have PoC or working code ready, could you please create short videos demonstrating the Dev Tools, Index Pattern, and sample data? This will help us visualize how they appear when implementing proposed change. We can request @kgcreative to review them.

@xinruiba xinruiba changed the title [MD] Removing "Local Cluster" option from datasource DropDown and Index Pattern creation page [MD] Abled to Enabled/Disable "Local Cluster" option from datasource DropDown and Index Pattern creation page Feb 2, 2024
@xinruiba xinruiba changed the title [MD] Abled to Enabled/Disable "Local Cluster" option from datasource DropDown and Index Pattern creation page [MD] Able to Enabled/Disable "Local Cluster" option from datasource DropDown and Index Pattern creation page Feb 2, 2024
@xinruiba
Copy link
Member Author

xinruiba commented Feb 5, 2024

Hey @kgcreative hope you are doing well.I have attached the POC demo under the description.
Please feel free to comments / raise any concerns.

Thanks

@kgcreative
Copy link
Member

This doesn't feel right to me, and i'm concerned about potential regressions with this behavior. I'm not sure that this should be a .yml setting either. All the OSD admin should do is enable the MDS feature. Everything else should be done in the UI.

  1. All of OSD is used to interacting with the host cluster of OSD (this is because OSD is build as a plug-in of the back-end)

  2. When MDS is enabled, we need a way to differentiate between the host cluster, and additional clusters.

  3. Doing this via YML feels heavy handed, since it requires cluster restarts for something that the customer will likely configure via UI. I would rather take care of showing a picker based on whether there are additional clusters or not.

My recommendation would be as follows:

When multiple cluster connections are present, we should expose the host cluster as a "MDS" connection in the data sources page. The customer should be able to 1) Rename the cluster, and 2) It should be set as default. This should, by default, either be called Host cluster, or Local cluster

The "Default cluster" should be a UI setting, similar to how you can set an index pattern as default. This is because the "Default" cluster should behave the same way that the current application works. This is to ensure plugins won't break if this feature is not present. We are working on figuring out a consistent cluster selection strategy for all plugins, but this should not be blocked by that.

Essentially, what we need is the following:

  1. Any Data Source should be able to be set as "Default" -- This should be indicated by aDefault badge next to that data source name
  2. If no data sources are set as "Default", then the local cluster should be set as default.
  3. If there is only one data source available, then we don't show the pickers, and the application's behavior is indistinguishable from MDS being disabled.
  4. Refer to the index pattern behavior as an example.

@xinruiba
Copy link
Member Author

xinruiba commented Feb 8, 2024

Hey @kgcreative, thanks for the comments.

For some context, this is the current behavior in AOS~

For your concerns:

First Concern:

  1. I'm not sure that this should be a .yml setting either.

Answer:

That's a good comment, We already take it into consideration.
This setting will be onboard to #5796 once it's ready and it will be a hot loading service. Since #5796 is still in progress, so we put this configuration under .yml file now.

Second Concern:

  1. Potential regressions with this behavior

Answer:

That's also a good point and all other recommendations values a lot.
Currently when MD is not enabled, all function will be the same.

When MD is enabled and this flag is enabled, as the first phase, we add an error handling to make sure dataSoruce should always exist #5751 and that change can protect us from regressions.

Once "consistent cluster selection strategy for all plugins" is ready, we can do a second phase to onboard this change.

@kgcreative
Copy link
Member

For what it's worth, for the first concern, i'm not concerned about dataSource.Enabled = True being a .yml setting, what i'm questioning is whether we need data_source.defaultCluster = True. I feel like that second one can be inferred simply by whether or not there is a host cluster present, and whether there are more than one data source enabled, and then we just do the right thing in the UI. Having to remember to flip a switch for default cluster is what i'm questioning here.

@xinruiba
Copy link
Member Author

xinruiba commented Feb 8, 2024

Thanks for the comments and that is a good point~

To answer the question shortly:
We don't need to to remember to flip a switch for default cluster, it will be by default enabled when that config is commented out in .yml file. (Change is here: #5811)
Thus it won't change any current behavior.

More Context
I have a discussion with @tianleh who is the owner of this RFC #5796 for the necessary of introducing this config data_source.defaultCluster in.

We want to keep that config because it can keep the flexibility to enable / disabled local cluster when MD is enabled.
And once that RFC #5796 is complete, our customer will be able to turn it on / off from the App settings.

@xinruiba
Copy link
Member Author

xinruiba commented Feb 9, 2024

Discussed with @kgcreative and we agree on:

  1. Rename the config name to "hideLocalCluster"to make the config more clear.
  2. Other changes will be good and act as the screenshots attached under the description.

@kgcreative please feel free to comments in case anything I miss. Thanks

@kgcreative
Copy link
Member

@xinruiba I'm aligned with this. hideLocalCluster is clear about the intent and is unambiguous

@Flyingliuhub
Copy link
Member

@xinruiba when you complete this one, could you please mark #5696 complete as well. Thanks

@xinruiba xinruiba changed the title [MD] Able to Enabled/Disable "Local Cluster" option from datasource DropDown and Index Pattern creation page [MD] Able to Hide "Local Cluster" option from datasource DropDown and Index Pattern creation page Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v2.13.0
Projects
None yet
Development

No branches or pull requests

4 participants