Skip to content

Conversation

@mawinter69
Copy link

For openstack the right BlobStore depends on the region. So it should be the
task of the provider to get the right BlobStore.
BlobStoreProvider is currently working with BlobStoreContext. But at all
places the context is just used to call the getBlobStore() method.

For openstack the right BlobStore depends on the region. So it should be the
task of the provider to get the right BlobStore.
BlobStoreProvider is currently working with BlobStoreContext. But at all
places the context is just used to call the getBlobStore() method.
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Is there an OpenStack implementation that would be downstream of this PR?

@mawinter69
Copy link
Author

yes, I'm working on an implementation for openstack. Though I would prefer having it in a separate plugin. But that requires #83 first.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Looks harmless. Awaiting downstream PR demonstrating usage. (See JEP-305 instructions.)

}
}

Copy link
Member

Choose a reason for hiding this comment

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

Better to revert changes to this file.

return context;
}

Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@jglick
Copy link
Member

jglick commented Sep 5, 2019

#83 is not likely to happen.

Assuming #96 lands, can https://docs.openstack.org/newton/config-reference/object-storage/configure-s3.html be used to access storage via the S3 emulation layer without code changes, or are there unimplemented features that this plugin relies on such as presigned URLs?

@mawinter69
Copy link
Author

mawinter69 commented Sep 12, 2019

In order to access swift you have to use signed urls, so I guess it can't be used.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Is there an OSS repo where you are prototyping the downstream implementation?


/** Creates the jclouds handle for working with blob. */
@NonNull
public abstract BlobStoreContext getContext() throws IOException;
Copy link
Member

Choose a reason for hiding this comment

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

This could perhaps be made nonabstract (throwing an AbstractMethodError at runtime if neither method is overridden, to avoid a StackOverflowError), since in the other direction there is a BlobStore.getContext() (used in this plugin only to close the context for tests…which is likely a bug).

Copy link
Author

Choose a reason for hiding this comment

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

I don't see where BlobStore.getContext() is used. In the test BlobStoreProvider.getContext() is used and on this context the close() is called.
I think it should stay abstract.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see where BlobStore.getContext() is used.

Sorry, to clarify: I think we could write something along the lines of:

public /* final */ BlobStoreContext getContext() throws IOException {
    if (Util.isOverridden(BlobStoreProvider.class, getClass(), "getBlobStore")) {
        return getBlobStore().getContext();
    } else {
        throw new AbstractMethodError("override getBlobStore");
    }
}
public /* abstract */ BlobStore getBlobStore() throws IOException {
    return getContext().getBlobStore();
}

where the modifiers could be uncommented and the code simplified if we did not care to retain backward compatibility for hypothetical external implementations (none are known).

Copy link
Author

Choose a reason for hiding this comment

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

ok, this might work as well, when speaking of not retaining backward compatibility one could also consider removing the getContext() method completely.
The getBlobStore should always be abstract I think.

Copy link
Member

Choose a reason for hiding this comment

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

when speaking of not retaining backward compatibility one could also consider removing the getContext() method completely

Yes, you could just inline it in that case.

The getBlobStore should always be abstract

Then calls to this method against a (hypothetical) existing external implementation, which overrode/implemented getContext, would fail with an AbstractMethodError.

The above paired method idiom is common in Jenkins sources, by the way, when we change our minds about what methods an implementation of some interface (or abstract class) should define. Somewhat cleaner is to just deprecate the whole “interface” and introduce, say, a BlobStoreProvider2 while having callers (anything enumerating this ExtensionList) convert from the old implementation.

Again, any of these tricks may just be overkill—the only other implementation of JEP-202 I know about is azure-artifact-manager, which uses the Azure APIs directly rather than via this plugin and jclouds.

@jglick
Copy link
Member

jglick commented Jan 9, 2020

For openstack the right BlobStore depends on the region.

Is this something that could be configured more generically? I see that there is a RegionScopedSwiftBlobStore and RegionScopedBlobStoreContext but I am not clear on how they are used.

@mawinter69
Copy link
Author

Opened another pull request (#111) that implements openstack swift

@jglick
Copy link
Member

jglick commented Mar 5, 2021

Given #111, I do not think this PR is still relevant.

@jglick jglick closed this Mar 5, 2021
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