You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Zack Shoylev <no...@github.com> on 2015/10/26 22:42:21 UTC
[jclouds] Fixes tests failing on windows filesystems (#871)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/871
-- Commit Summary --
* Fixes tests failing on windows filesystems
-- File Changes --
M apis/filesystem/src/main/java/org/jclouds/filesystem/config/FilesystemBlobStoreContextModule.java (5)
M apis/filesystem/src/main/java/org/jclouds/filesystem/util/Utils.java (26)
M apis/filesystem/src/test/java/org/jclouds/filesystem/FilesystemBlobStoreTest.java (5)
M apis/filesystem/src/test/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImplTest.java (4)
M blobstore/src/test/java/org/jclouds/blobstore/integration/internal/BaseBlobIntegrationTest.java (2)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/871.patch
https://github.com/jclouds/jclouds/pull/871.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Zack Shoylev <no...@github.com>.
> @@ -49,7 +55,23 @@ public static void deleteRecursively(File file) throws IOException {
> }
> }
> }
> - Files.delete(file.toPath());
> +
I should have explained further. Yes, this PR is mostly to make the tests compatible with virtually any windows virus checker.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r43131563
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Zack Shoylev <no...@github.com>.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +package org.jclouds.util;
> +
> +public class Utils {
I think it has to be main branch, as we use this during configuration?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r44424890
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Phillips <no...@github.com>.
> @@ -477,8 +478,10 @@ public String putBlob(final String containerName, final Blob blob) throws IOExce
> return base16().lowerCase().encode(actualHashCode.asBytes());
> } catch (IOException ex) {
> if (outputFile != null) {
> - if (!outputFile.delete()) {
> - logger.debug("Could not delete %s", outputFile);
> + try {
> + delete(outputFile);
> + } catch (IOException e) {
> + logger.debug("Could not delete %s", e);
Wrong log message now? `("Could not delete %s: %s", outputFile, e)` perhaps?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r44410564
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Zack Shoylev <no...@github.com>.
@demobox @andrewgaul Updated again. Should be good to merge? Hopefully I didn't miss anything, it's a bit larger in scope than originally planned.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871#issuecomment-155569257
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Phillips <no...@github.com>.
> }
> }
>
> + try {
> + delete(fileToBeDeleted);
> + } catch (IOException e) {
> + logger.debug("%s", e);
Same comment about the log message
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r44410642
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Gaul <no...@github.com>.
> @@ -491,6 +495,7 @@ public void testPutDirectoryBlobs() {
> assertTrue(blobStore.blobExists(CONTAINER_NAME, childKey));
>
> blobStore.removeBlob(CONTAINER_NAME, parentKey);
> + Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
Could we change `removeBlob` to loop until the blob is deleted to give user-facing consistency instead of just test consistency?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r43336766
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Gaul <no...@github.com>.
> addBlobToContainer(container, blob);
> + awaitConsistency();
If we change `removeBlob` will this again look strongly-consistent? Also do we have any issues with overwriting blobs?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r43336790
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Phillips <no...@github.com>.
> + for (int n = 0; n < 10; n++) {
> + try {
> + Files.delete(file.toPath());
> + } catch (DirectoryNotEmptyException dnee) {
> + // A previous file delete operation did not finish before this call
> + Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
> + continue;
> + } catch (AccessDeniedException ade) {
> + // The file was locked by antivirus, indexing, or another operation triggered by previous file modification
> + Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
> + continue;
> + } catch (NoSuchFileException nse) {
> + //if (n == 0) throw nse; // The file was deleted elsewhere or the file was not found
> + return; // The file has been eventually deleted after a previous operation that failed. no-op
> + }
> + return;
[minor] Move this to directly after `Files.delete`, for clarity?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r44410829
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Zack Shoylev <no...@github.com>.
Rebased, will merge and we might address further changes in my next PR (already working on it)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871#issuecomment-155907750
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Phillips <no...@github.com>.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +package org.jclouds.util;
> +
> +public class Utils {
> I think it has to be main branch, as we use this during configuration?
Ah, in the module, indeed. But since we're not using this anywhere outside this provider (unless I'm missing something), keep them in the `filesystem.Utils`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r44427134
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Zack Shoylev <no...@github.com>.
Closed #871.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871#event-462017233
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Zack Shoylev <no...@github.com>.
> addBlobToContainer(container, blob);
> + awaitConsistency();
You can change removeBlob to be strongly consistent - but I didn't want to change the behavior (as it would increase the scope of the PR).
The way to do that would be to abstract deletion better and use that in removeBlob as well as other places (such as cleaning up the empty directory tree).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r43343119
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Gaul <no...@github.com>.
> @@ -49,7 +55,23 @@ public static void deleteRecursively(File file) throws IOException {
> }
> }
> }
> - Files.delete(file.toPath());
> +
This doesn't seem right -- usually a virus checker or similar is holding open a file descriptor if you cannot immediately delete on Windows.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r43062225
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Zack Shoylev <no...@github.com>.
> @@ -161,14 +164,6 @@ public void testDeleteDirectory() throws IOException {
> TestUtils.directoryExists(TARGET_CONTAINER_NAME, false);
> }
>
> - public void testDeleteDirectory_ErrorWhenNotExists() {
Yes, had to slightly change the spec.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r44434149
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Phillips <no...@github.com>.
> @@ -49,7 +55,23 @@ public static void deleteRecursively(File file) throws IOException {
> }
> }
> }
> - Files.delete(file.toPath());
> +
@zack-shoylev Do you know what the underlying issue is here? Waiting for locks to be released, or..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r43058785
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Phillips <no...@github.com>.
> @@ -786,8 +792,10 @@ private void removeDirectoriesTreeOfBlobKey(String container, String blobKey) {
> File directory = new File(buildPathStartingFromBaseDir(container, parentPath));
> String[] children = directory.list();
> if (null == children || children.length == 0) {
> - if (!directory.delete()) {
> - logger.debug("Could not delete %s", directory);
> + try {
> + delete(directory);
> + } catch (IOException e) {
> + logger.debug("%s", e);
Same comment about the log message
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r44410691
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Phillips <no...@github.com>.
> @@ -39,7 +39,10 @@
> protected void configure() {
> bind(BlobStore.class).to(LocalBlobStore.class);
> install(new BlobStoreObjectModule());
> - bind(ConsistencyModel.class).toInstance(ConsistencyModel.STRICT);
> + if (System.getProperty("os.name", "").toLowerCase().contains("windows"))
Use [filesystem's `TestUtils`](https://github.com/jclouds/jclouds/blob/master/apis/filesystem/src/test/java/org/jclouds/filesystem/utils/TestUtils.java#L224)? Or, better still, move that method to the [core `TestUtils` class](https://github.com/jclouds/jclouds/blob/master/core/src/test/java/org/jclouds/utils/TestUtils.java)?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r43059026
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Gaul <no...@github.com>.
> + for (int n = 0; n < 10; n++) {
> + try {
> + Files.delete(file.toPath());
> + } catch (DirectoryNotEmptyException dnee) {
> + Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
> + continue;
> + } catch (AccessDeniedException ade) {
> + Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
> + continue;
> + } catch (NoSuchFileException nse) {
> + if (n == 0) throw nse;
> + return;
> + }
> + return;
> + }
> + throw new RuntimeException("Could not delete: " + file.toPath());
Can we abstract this logic out to a method, including Javadoc explaining the motivations? Also it seems like we should call this helper in `removeBlob` as well.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r43336697
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Zack Shoylev <no...@github.com>.
> @@ -39,7 +39,10 @@
> protected void configure() {
> bind(BlobStore.class).to(LocalBlobStore.class);
> install(new BlobStoreObjectModule());
> - bind(ConsistencyModel.class).toInstance(ConsistencyModel.STRICT);
> + if (System.getProperty("os.name", "").toLowerCase().contains("windows"))
> + bind(ConsistencyModel.class).toInstance(ConsistencyModel.EVENTUAL);
Agreed, I am mostly exploring options on how to fix the tests on a windows system with a virus checker (very standard setup).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r43131657
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Phillips <no...@github.com>.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +package org.jclouds.util;
> +
> +public class Utils {
Do we need this in the main branch, or can we keep this under `src/test`..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r44411112
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Phillips <no...@github.com>.
> Should be good to merge?
Looks good to me, @zack-shoylev - +1. Thanks for the updates!
@andrewgaul may have some comments on regarding Windows as an eventually consistent store..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871#issuecomment-155611018
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Zack Shoylev <no...@github.com>.
@andrewgaul @demobox just updated this to cover the notes; i also spent some time figuring out converting to a fully transactional solution, but doesn't seem to be really needed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871#issuecomment-155261801
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Zack Shoylev <no...@github.com>.
merged
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871#issuecomment-155983551
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Phillips <no...@github.com>.
> @@ -161,14 +164,6 @@ public void testDeleteDirectory() throws IOException {
> TestUtils.directoryExists(TARGET_CONTAINER_NAME, false);
> }
>
> - public void testDeleteDirectory_ErrorWhenNotExists() {
No longer applicable or..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r44410933
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Phillips <no...@github.com>.
> @@ -477,8 +478,10 @@ public String putBlob(final String containerName, final Blob blob) throws IOExce
> return base16().lowerCase().encode(actualHashCode.asBytes());
> } catch (IOException ex) {
> if (outputFile != null) {
> - if (!outputFile.delete()) {
> - logger.debug("Could not delete %s", outputFile);
> + try {
> + delete(outputFile);
[nit] Indent. Again, can clean up during the merge.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r44410444
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Gaul <no...@github.com>.
> @@ -39,7 +39,10 @@
> protected void configure() {
> bind(BlobStore.class).to(LocalBlobStore.class);
> install(new BlobStoreObjectModule());
> - bind(ConsistencyModel.class).toInstance(ConsistencyModel.STRICT);
> + if (System.getProperty("os.name", "").toLowerCase().contains("windows"))
> + bind(ConsistencyModel.class).toInstance(ConsistencyModel.EVENTUAL);
I wouldn't call a local file system eventually consistent!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r43062294
Re: [jclouds] Fixes tests failing on windows filesystems (#871)
Posted by Andrew Phillips <no...@github.com>.
> @@ -39,7 +41,12 @@
> protected void configure() {
> bind(BlobStore.class).to(LocalBlobStore.class);
> install(new BlobStoreObjectModule());
> - bind(ConsistencyModel.class).toInstance(ConsistencyModel.STRICT);
> + if (isWindows()) {
> + bind(ConsistencyModel.class).toInstance(ConsistencyModel.EVENTUAL);
> + }
> + else {
[nit] Formatting (`else` on the same line?). We can do this during the merge, though - no need to update the PR, from my perspective.
@gaul: Your comment about regarding as eventually consistent - does this work for you now?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/871/files#r44410350