Skip to content

Do not create directory on readonly repository (#21495) #26909

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
Merged
merged 1 commit into from
Nov 3, 2017
Merged

Do not create directory on readonly repository (#21495) #26909

merged 1 commit into from
Nov 3, 2017

Conversation

@liketic
Copy link

@liketic liketic commented Oct 6, 2017 •
edited
Loading

Closes #21495

For FsBlobStore and HdfsBlobStore, if repository is read only, we can aware the read only setting and do not create directories if readonly is true.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 6, 2017

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@rjernst rjernst requested a review from ywelsch October 12, 2017 21:26
@rjernst rjernst added :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >bug review labels Oct 12, 2017
@ywelsch
Copy link
Contributor

ywelsch commented Oct 27, 2017

Hi @liketic. Sorry for the late reply here. On the issue #21495 that I opened, I was thinking about a slightly different solution: That we should take the "readonly" setting of the repository into account.
If a repository is marked as readonly, we should never write to that repository. If it's not marked as readonly, we could possibly allow writes to the repository when a restore happens.
Regarding this PR, I'm not sure whether we should associate the "readOnly" property with the BlobPath object. The FsBlobStore object could be made aware of the repository-level "readOnly" setting instead (we already pass the settings). WDYT?

@liketic liketic force-pushed the bugfix/issues/21495 branch from 74c3432 to db47b9a Compare October 27, 2017 15:43
@liketic
Copy link
Author

liketic commented Oct 27, 2017

@ywelsch Really appreciated for your review. It's OK to me to read readonly from repository settings. I did the change and please review again. Thanks in advance. 😄

@ywelsch
Copy link
Contributor

ywelsch commented Nov 2, 2017

@elasticmachine test this please

@ywelsch
Copy link
Contributor

ywelsch commented Nov 2, 2017

@liketic Can you update the PR title and add a description that says what's been done here (i.e. Do not create directory on readonly repository)?

Note that HdfsBlobStore has the same issue (it does an mkdirs() independently of readonly flag). Would you like to fix it there as well?

@liketic liketic changed the title Do not create directory when reading snapshot (#21495) Do not create directory on readonly repository (#21495) Nov 3, 2017
@liketic
Do not create directories if repository is readonly (elastic#21495)
489bd71 
For FsBlobStore and HdfsBlobStore, if the repository is read only, the blob store should aware the read only setting and do not create directories even it's not exists.

Do not mkdirs if HDFS repository is read only
@liketic liketic force-pushed the bugfix/issues/21495 branch from db47b9a to 489bd71 Compare November 3, 2017 02:06
@liketic
Copy link
Author

liketic commented Nov 3, 2017 •
edited
Loading

@ywelsch Thanks for your comment! I rebased and pushed 489bd71 to make the test can be passed. Hope it makes sense. 😄

@ywelsch
Copy link
Contributor

ywelsch commented Nov 3, 2017

@elasticmachine retest this please

@ywelsch ywelsch merged commit 0f21262 into elastic:master Nov 3, 2017
ywelsch pushed a commit that referenced this pull request Nov 3, 2017
@ywelsch
Do not create directories if repository is readonly (#26909)
4fe0f79 
For FsBlobStore and HdfsBlobStore, if the repository is read only, the blob store should be aware of the readonly setting and do not create directories if they don't exist.

Closes #21495
ywelsch pushed a commit that referenced this pull request Nov 3, 2017
@ywelsch
Do not create directories if repository is readonly (#26909)
84ef95f 
For FsBlobStore and HdfsBlobStore, if the repository is read only, the blob store should be aware of the readonly setting and do not create directories if they don't exist.

Closes #21495
@ywelsch
Copy link
Contributor

ywelsch commented Nov 3, 2017

Thanks for this PR, @liketic. I'll backport this to the appropriate branches.
For future reference, don't rebase PRs (instead merge latest master into the PR branch). This will help us quickly identify what's changed since the last code review.

@ywelsch ywelsch removed the review label Nov 3, 2017
@liketic
Copy link
Author

liketic commented Nov 3, 2017

@ywelsch I got it! Thanks for your help! I will never rebase unless reviewier ask me to.

@liketic liketic deleted the bugfix/issues/21495 branch November 3, 2017 13:10
martijnvg added a commit that referenced this pull request Nov 4, 2017
@martijnvg
Merge remote-tracking branch 'es/master' into ccr
e61e4b8 
* es/master:
  Fix snapshot getting stuck in INIT state (#27214)
  Add an example of dynamic field names (#27255)
  #26260 Allow ip_range to accept CIDR notation (#27192)
  #27189 Fixed rounding of bounds in scaled float comparison (#27207)
  Add support for Gradle 4.3 (#27249)
  Fixes QueryStringQueryBuilderTests
  build: Fix setting the incorrect bwc version in mixed cluster qa module
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery BWC
  Adjust assertions for sequence numbers BWC tests
  Do not create directories if repository is readonly (#26909)
  [Test] Fix InternalStatsTests
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery
  Uses norms for exists query if enabled (#27237)
  Reinstate recommendation for ≥ 3 master-eligible nodes. (#27204)
martijnvg added a commit that referenced this pull request Nov 4, 2017
@martijnvg
Merge branch '6.x' into ccr-6.x
3ed5aaa 
* 6.x:
  Add an example of dynamic field names (#27255)
  fixed checkstyle violation
  #26260 Allow ip_range to accept CIDR notation (#27192)
  #27189 Fixed rounding of bounds in scaled float comparison (#27207)
  [TEST] Fix failing exists query test
  test: Do not run old parent/child tests against a cluster with minimum version greater than 6.0.0
  Add support for Gradle 4.3 (#27249)
  Fixes QueryStringQueryBuilderTests
  build: Fix setting the incorrect bwc version in mixed cluster qa module
  fix compil after backport
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery BWC
  Adjust assertions for sequence numbers BWC tests
  Do not create directories if repository is readonly (#26909)
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery
  Uses norms for exists query if enabled (#27237)
  Reinstate recommendation for ≥ 3 master-eligible nodes. (#27204)
@DaveCTurner DaveCTurner mentioned this pull request Apr 9, 2019
Closed
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Apr 16, 2019
@DaveCTurner
Do not create missing directories in readonly repo
967d2bf 
Today we erroneously look for a node setting called `readonly` when deciding
whether or not to create a missing directory in a filesystem repository. This
change fixes this by using the repository setting instead.

Closes elastic#41009
Relates elastic#26909
@DaveCTurner DaveCTurner mentioned this pull request Apr 16, 2019
Merged
DaveCTurner added a commit that referenced this pull request Apr 17, 2019
@DaveCTurner
Do not create missing directories in readonly repo (#41249)
dc0599a 
Today we erroneously look for a node setting called `readonly` when deciding
whether or not to create a missing directory in a filesystem repository. This
change fixes this by using the repository setting instead.

Closes #41009
Relates #26909
DaveCTurner added a commit that referenced this pull request Apr 17, 2019
@DaveCTurner
Do not create missing directories in readonly repo (#41249)
bfa06d9 
Today we erroneously look for a node setting called `readonly` when deciding
whether or not to create a missing directory in a filesystem repository. This
change fixes this by using the repository setting instead.

Closes #41009
Relates #26909
DaveCTurner added a commit that referenced this pull request Apr 17, 2019
@DaveCTurner
Do not create missing directories in readonly repo (#41249)
6aa95d8 
Today we erroneously look for a node setting called `readonly` when deciding
whether or not to create a missing directory in a filesystem repository. This
change fixes this by using the repository setting instead.

Closes #41009
Relates #26909
DaveCTurner added a commit that referenced this pull request Apr 17, 2019
@DaveCTurner
Do not create missing directories in readonly repo (#41249)
afbd278 
Today we erroneously look for a node setting called `readonly` when deciding
whether or not to create a missing directory in a filesystem repository. This
change fixes this by using the repository setting instead.

Closes #41009
Relates #26909
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
@DaveCTurner
Do not create missing directories in readonly repo (elastic#41249)
a5aad00 
Today we erroneously look for a node setting called `readonly` when deciding
whether or not to create a missing directory in a filesystem repository. This
change fixes this by using the repository setting instead.

Closes elastic#41009
Relates elastic#26909
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ywelsch ywelsch ywelsch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

>bug :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v6.0.1 v6.1.0 v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@liketic @elasticmachine @ywelsch @colings86 @rjernst