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

[US2662] check if underlying filesystem supports extents mapping. #105

Merged
merged 3 commits into from
Aug 28, 2018
Merged

[US2662] check if underlying filesystem supports extents mapping. #105

merged 3 commits into from
Aug 28, 2018

Conversation

utkarshmani1997
Copy link
Contributor

On branch US2662-verify-file-extents-mapping
Changes to be committed:
modified: ci/start_init_test.sh
modified: replica/replica.go

  1. Why is this change necessary?
  • To verify whether the underlying filesystem supports extents mapping.
  1. How does it address the issue?
  • In the earlier code we were ignoring the error which verifies if the
    filesystem supports extents mapping. So now we will catch the error
    and error out the replica.
  1. What side effects does this change have?
  • None.
  1. How to verify this change?
  • ci is already added to verify the changes.
  1. Any specific message for reviewer ?
  • No

Signed-off-by: Utkarsh Mani Tripathi [email protected]

 On branch US2662-verify-file-extents-mapping
 Changes to be committed:
	modified:   ci/start_init_test.sh
	modified:   replica/replica.go

1. Why is this change necessary?
-  To verify whether the underlying filesystem supports extents mapping.

2. How does it address the issue?
-  In the earlier code we were ignoring the error which verifies if the
   filesystem supports extents mapping. So now we will catch the error
   and error out the replica.

3. What side effects does this change have?
-  None.

4. How to verify this change?
-  ci is already added to verify the changes.

5. Any specific message for reviewer ?
-  No

Signed-off-by: Utkarsh Mani Tripathi <[email protected]>
Signed-off-by: Utkarsh Mani Tripathi <[email protected]>
Signed-off-by: Utkarsh Mani Tripathi <[email protected]>
@payes payes self-requested a review August 27, 2018 14:20
@@ -224,7 +224,11 @@ func construct(readonly bool, size, sectorSize int64, dir, head string, backingF
r.insertBackingFile()
r.ReplicaType = replicaType

PreloadLunMap(&r.volume)
if err := PreloadLunMap(&r.volume); err != nil {
logrus.Error("underlying file system does not support extent mapping")
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets also print the actual error received.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are returning the error to the callee, callee is printing the error : "operation not permitted". So printing the error here will pollute the logs unneccessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

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

Successfully merging this pull request may close these issues.

2 participants