-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Hide remote reindex migration behind feature flag #20636
base: master
Are you sure you want to change the base?
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.
Good solution, but there is still a part missing. This will leave users on Elasticsearch stuck in the wizard with no info why they can't proceed.
We either need to add a warning in the beginning that migration of data for these versions isn't supported and remove the guard or move the guard to the beginning of the wizard and also let them know in the UI.
And both the guard and the UI warning need to be aware of the feature flag
@moesterheld what about hiding the migration button/menu completely if you are on elasticsearch? |
@todvora Hiding the tab completely would work, but would also take them the ability to move to data node without migrating any data. Some people might be happy to start with an empty Opensearch in data node. |
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.
The backend part from @moesterheld looks good. I can't approve my own PR, but I tested it again locally.
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.
Code-wise it looks good, one small comment and the Reviewbot also has some remarks. But don't want to block it because of that.
<p>You can get more information on the Data Node migration <DocumentationLink page="graylog-data-node" text="documentation" />.</p> | ||
<br /> | ||
<MigrationDatanodeList /> | ||
{!(isElasticsearch && !isRemoteReindexingEnabled) && ( |
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.
{!(isElasticsearch && !isRemoteReindexingEnabled) && ( | |
{(!isElasticsearch && !isRemoteReindexingEnabled) && ( |
makes it clearer I think
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.
that's not a typo it's the negation of (isElasticsearch && !isRemoteReindexingEnabled)
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.
@grotlue it looks like that changes the logic
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.
Checked frontend functionality for all cases. LGTM
Looks good 👍 |
Description
Remote reindex migration will be enabled only when
remote_reindex_migration
is set toon
We will then add the following changes:
For all users:
For users with incompatible versions:
For users with compatible versions:
Motivation and Context
Implements https://github.com/Graylog2/graylog-plugin-enterprise/issues/8811
How Has This Been Tested?
Manually, switching the feature flag
Types of changes
Checklist: