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

Migrate registry from previous incorrect path #10486

Merged
merged 8 commits into from
Feb 5, 2019

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Feb 1, 2019

In 6.x Journalbeat placed its registry file under the wrong path ignoring the data.path settings.
This patch lets users migrate from registries under such paths when upgrading from 6.x to 7.x.

@kvch kvch requested review from ph and urso February 1, 2019 14:01
@kvch kvch requested a review from a team as a code owner February 1, 2019 14:01
@kvch kvch removed the request for review from a team February 1, 2019 14:01
@kvch kvch force-pushed the backward-compatibility-journalbeat branch from 71aceda to 8060df4 Compare February 4, 2019 15:06
@kvch kvch added the needs_backport PR is waiting to be backported to other branches. label Feb 4, 2019
@kvch kvch force-pushed the backward-compatibility-journalbeat branch from 8060df4 to a9d4955 Compare February 4, 2019 16:18
return err
}

err = helper.SafeFileRotate(c.file, c.file+".bak")
Copy link

Choose a reason for hiding this comment

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

We should close the files before calling SafeFileRotate. Also check that Close does not error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Donz.


err = f.Close()
if err != nil {
return fmt.Errorf("error closing old registry file: %+v", err)
Copy link

Choose a reason for hiding this comment

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

you still need to close target if this one fails.'

Why do we need to copy anyways? Isn't SafeFileRotate in order to move the file good enough?

The use of SafeFileRotate suggests a backup file is written and then rotated to c.file. When is c.file+".bak" written?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a mistake from me. I removed it, because it was unnecessary.

err = target.Close()
if err != nil {
return fmt.Errorf("error closing new registry file: %+v", err)
}
Copy link

Choose a reason for hiding this comment

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

the copy is not 'safe' without extra call to target.Sync(). Some filesystems (without journaling) also require a sync operation on the parent directory in order to sync file + directory meta data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Donz.

@kvch kvch force-pushed the backward-compatibility-journalbeat branch from b9b2f74 to 6fc47dd Compare February 5, 2019 21:39
@kvch kvch force-pushed the backward-compatibility-journalbeat branch from 6166aaf to 5c73b8e Compare February 5, 2019 21:52
pf, _ := os.Open(p)
if err != nil {
return nil
}
Copy link

Choose a reason for hiding this comment

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

nit; pf, err := os.Open(p).

@kvch
Copy link
Contributor Author

kvch commented Feb 5, 2019

All relevant tests are green.

@kvch kvch merged commit 9499811 into elastic:master Feb 5, 2019
kvch added a commit to kvch/beats that referenced this pull request Feb 5, 2019
In 6.x Journalbeat placed its registry file under the wrong path ignoring the data.path settings.
This patch lets users migrate from registries under such paths when upgrading from 6.x to 7.x.
(cherry picked from commit 9499811)
@kvch kvch added v6.7.0 and removed needs_backport PR is waiting to be backported to other branches. labels Feb 5, 2019
kvch added a commit to kvch/beats that referenced this pull request Feb 5, 2019
In 6.x Journalbeat placed its registry file under the wrong path ignoring the data.path settings.
This patch lets users migrate from registries under such paths when upgrading from 6.x to 7.x.
(cherry picked from commit 9499811)
@kvch kvch added the v6.6.1 label Feb 5, 2019
kvch added a commit that referenced this pull request Feb 6, 2019
…ath (#10597)

In 6.x Journalbeat placed its registry file under the wrong path ignoring the data.path settings.
This patch lets users migrate from registries under such paths when upgrading from 6.x to 7.x.
(cherry picked from commit 9499811)
urso pushed a commit that referenced this pull request Feb 6, 2019
In 6.x Journalbeat placed its registry file under the wrong path ignoring the data.path settings.
This patch lets users migrate from registries under such paths when upgrading from 6.x to 7.x.
(cherry picked from commit 9499811)
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
In 6.x Journalbeat placed its registry file under the wrong path ignoring the data.path settings.
This patch lets users migrate from registries under such paths when upgrading from 6.x to 7.x.
DStape pushed a commit to DStape/beats that referenced this pull request Aug 20, 2019
…rrect path (elastic#10597)

In 6.x Journalbeat placed its registry file under the wrong path ignoring the data.path settings.
This patch lets users migrate from registries under such paths when upgrading from 6.x to 7.x.
(cherry picked from commit 9499811)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…c#10598)

In 6.x Journalbeat placed its registry file under the wrong path ignoring the data.path settings.
This patch lets users migrate from registries under such paths when upgrading from 6.x to 7.x.
(cherry picked from commit 388b4da)
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.

2 participants