-
Notifications
You must be signed in to change notification settings - Fork 220
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
Add middleware to override storage class. #666
base: master
Are you sure you want to change the base?
Conversation
@michaelcourcy could you take a look at this to see if it meets your needs? |
Hi @gaul, is there any reason that you chose the word
Maybe tier relates to something else, something more global that make sense in the context of s3proxy ? |
Ok I understand S3 is a more specific use case of jcloud.
|
Using |
Ah I missed the mapping code
It's already a lot and far enough for starting this new feature IMO. I will try to test it against AWS S3 and then later against CEPH. It's been a long time I haven't build a maven project. |
Thinking about this some more, anything that requires reading the source code is a non-starter. I think this would work better if the user specifies the desired storage class in S3 terms and S3Proxy should log when it does a lossy conversion. If we round-trip the StorageClass -> Tier -> StorageClass we have lossless conversions for:
And lossy conversions for:
Logging the conversions makes even more sense for non-S3 object stores. But I imagine most users will use S3 and be happy with the lossless mapping of the first four storage classes. |
If user proxy against a s3 for sure but if he proxies against Azure or GCP then this s3 mapping may be confusing and against the spirit of the project ? (I'm asking I'm really not sure)
Sure but we'll add a wiki page |
@gaul My test fails but maybe I missed the configuration. I checkout tier-blobstore branch and build
s3proxy.properties I introduce
launch s3proxy then check my configuration works
And try to put a file
But when I check the storageclass of test.txt it's STANDARD instead of STANDARD_IA
I get
|
Also tried with
But did not work. |
Sorry please try the latest proposed PR -- previously it lacked the changes to Main.java to actually accept the configuration. |
It works now !
|
I will test it against CEPH S3. I created this wiki page to document it https://github.com/gaul/s3proxy/wiki/Middleware-Tier-blobstore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except the little change I suggest in the comments but not a blocker IMO.
* s3proxy.tier-blobstore = VALUE | ||
* | ||
* VALUE can be anything from org.jclouds.blobstore.domain.Tier, e.g., | ||
* STANRDARD, COOL, COLD, ARCHIVE. org.jclouds.s3.domain.StorageClass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I use COOL in the configuration
s3proxy.tier-blobstore=COOL
I get this error message
Exception in thread "main" java.lang.IllegalArgumentException: No enum constant org.jclouds.blobstore.domain.Tier.COOL
at java.base/java.lang.Enum.valueOf(Enum.java:293)
at org.jclouds.blobstore.domain.Tier.valueOf(Tier.java:24)
at org.gaul.s3proxy.Main.parseMiddlewareProperties(Main.java:278)
at org.gaul.s3proxy.Main.main(Main.java:125)
Look like we are limited by the enum values of org.jclouds.blobstore.domain.Tier which are
STANDARD
INFREQUENT
ARCHIVE
I would suggest to change this comment accordingly unless I pick up the wrong version of jcloud in the maven dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely that's what happened in this version there is COOL and COLD
https://github.com/apache/jclouds/blob/master/blobstore/src/main/java/org/jclouds/blobstore/domain/Tier.java
I did not figure out why when I built s3proxy but that does not matter please ignore my comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jclouds 2.6.0 only has a few Tier constants but 2.7.0-SNAPSHOT includes a wider variety.
This now maps and logs S3 storage classes instead of jclouds Tiers as previously discussed. |
blobStore = storageClassBlobStore; | ||
System.err.println("Requested tier: tier"); | ||
System.err.println("Overriden to: " + | ||
StorageClass.fromTier(storageClassBlobStore.getTier())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only works for S3-S3 mappings and is confusing for other storage backends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the wiki accordingly.
This is best-effort and some storage classes do not map exactly, particularly for non-S3 object stores. Fixes #625.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me! Unfortunate that we're limited by the jclouds set of storage classes.
This is best-effort and some storage classes do not map exactly, particularly for non-S3 object stores. Fixes #625.