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

show Journal Size in In-Place Migration steps #18492

Merged
merged 4 commits into from Mar 6, 2024
Merged

Conversation

gally47
Copy link
Contributor

@gally47 gally47 commented Mar 5, 2024

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@gally47 gally47 self-assigned this Mar 5, 2024
@gally47 gally47 requested a review from ousmaneo March 5, 2024 17:18
Copy link
Contributor

@ousmaneo ousmaneo left a comment

Choose a reason for hiding this comment

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

Looks good, I added some suggestion that we should take care of.

fetchJournalDowntimeSize,
{
onError: (errorThrown) => {
UserNotification.error(`Loading Data Node failed with status: ${errorThrown}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
UserNotification.error(`Loading Data Node failed with status: ${errorThrown}`,
UserNotification.error(`Loading Data Node migration journal estimate failed with status: ${errorThrown}`,

{
onError: (errorThrown) => {
UserNotification.error(`Loading Data Node failed with status: ${errorThrown}`,
'Could not load Data Node');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'Could not load Data Node');
'Could not load Data Node journal size estimate.');

<h3>Journal downtime size warning</h3>
<p>Please note that during migration data processing will stop on your Graylog node, this will result in the journal growing in size.</p>
<p>Therefore increase your journal volume size.</p>
<p>Size: <b>{data} KB/min</b></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<p>Size: <b>{data} KB/min</b></p>
<p>Your current journal throughput is: <b>{data} KB/min</b></p>

</>
);
const JournalDowntimeWarning = ({ currentStep, onTriggerStep }: MigrationStepComponentProps) => {
const { data } = useJournalDowntimeSize();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also handle displaying a message when there is an error when getting the throughput estimation.

@gally47 gally47 requested a review from ousmaneo March 6, 2024 09:20
Copy link
Contributor

@ousmaneo ousmaneo left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@moesterheld moesterheld left a comment

Choose a reason for hiding this comment

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

lgtm

if we get feedback from testers about how long they took on average for the restart, we could add a estimate for the journal size that multiplies this values by the needed number of minutes (plus buffer)

@gally47 gally47 merged commit c4afe7d into master Mar 6, 2024
6 checks passed
@gally47 gally47 deleted the show-journal-size branch March 6, 2024 12:27
janheise pushed a commit that referenced this pull request Mar 7, 2024
* show JournalDowntimeSize

* changelog file

* fix review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants