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

test: Fix TestDropReadOnly failed on windows #661

Merged
merged 1 commit into from Jan 4, 2019

Conversation

Projects
None yet
4 participants
@Xuanwo
Copy link
Contributor

Xuanwo commented Jan 2, 2019

This change is Reviewable

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jan 2, 2019

CLA assistant check
All committers have signed the CLA.

@Xuanwo

This comment has been minimized.

Copy link
Contributor

Xuanwo commented Jan 2, 2019

I's not so sure for the problem on Munmap, maybe we should keep the handle like here: https://github.com/edsrzf/mmap-go/blob/master/mmap_windows.go#L69-L72

@martinmr
Copy link
Contributor

martinmr left a comment

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Xuanwo)


managed_db_test.go, line 161 at r1 (raw file):

	// acquireDirectoryLock will return ErrWindowsNotSupported on Windows platform, we can ignore it safely.

nitpick: change the comment to

// acquireDirectoryLock returns ErrWindowsNotSupported on Windows. It can be ignored safely.

Comments should preferably be written in the present tense.

@manishrjain

This comment has been minimized.

Copy link
Member

manishrjain commented Jan 2, 2019

The bigger question is, why does this fail on Windows? vlog.deleteLogFile does call unmap before closing the file and deleting it.

@martinmr : Can you figure out the cause of this failure, and fix it?

@Xuanwo Xuanwo force-pushed the Xuanwo:test_fix branch from 9d84cc3 to dd67108 Jan 3, 2019

@Xuanwo

This comment has been minimized.

Copy link
Contributor

Xuanwo commented Jan 3, 2019

Well, it makes sense. I force pushed a commit to do the change, PLTA @martinmr

@martinmr

This comment has been minimized.

Copy link
Contributor

martinmr commented Jan 3, 2019

Thanks for the change. I'll first take a look at badger to see if the issue can be fixed without adding os-specific code in the tests.

@Xuanwo

This comment has been minimized.

Copy link
Contributor

Xuanwo commented Jan 3, 2019

OK, it's better if no os-specific code needed.

@martinmr

This comment has been minimized.

Copy link
Contributor

martinmr commented Jan 3, 2019

I checked and this looks like a limitation of the file system so it should be fine to have the OS-specific check since there's no way around it.

@manishrjain, what do you say?

@martinmr
Copy link
Contributor

martinmr left a comment

:lgtm:

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @martinmr and @Xuanwo)

@martinmr

This comment has been minimized.

Copy link
Contributor

martinmr commented Jan 4, 2019

Thanks for the PR. We discussed this issue internally and since there's no easy way to get around this limitation, we'll merge your PR.

@martinmr martinmr merged commit 984037c into dgraph-io:master Jan 4, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
code-review/reviewable 1 file, 1 discussion left (martinmr, Xuanwo)
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment