Use ConcurrentSkipListMap for transient blobstore. #1565
Conversation
|
+1 |
|
Adrian Cole » jclouds #51 FAILURE |
|
jclouds-java-7-pull-requests #404 FAILURE |
|
Needs a similar change in filesystem blobstore, although note that Java 6 does not support resumable readdir. |
|
@adriancole : I like the idea of having overloaded getBlobKeysInsideContainer(). If callers don't care they need not pass the second argument. But I'd like callers to have the ability to pass null (in which case we fallback to using single-argument implementation). For e.g. the list() method in LocalAsyncBlobStore can pass options.getMarker() which may or may not be null. Also, is it okay to do the renaming in a separate commit? It'll be easier to keep this one specific to bug 1561. @andrewgaul : I don't understand what the filesystem blobstore has to do with BlockUntilInitScriptStatusIsZeroThenReturnOutputTest.testloopUntilTrueOrThrowCancellationExceptionReturnsWhenPredicateIsTrueSecondTimeWhileNotCancelled(). I ran this test locally and did not hit any assert. |
|
well, typically we don't like null params at all. In the rare case we do, On Wed, Apr 24, 2013 at 2:24 PM, Shri Javadekar [email protected]:
|
|
Just to repeat: same compilation failure in both builds and @shrinandj I think that's what @andrewgaul might be referring to... |
|
Adrian Cole » jclouds #53 SUCCESS |
|
jclouds-java-7-pull-requests #406 SUCCESS |
|
just helping answer this, as I can't necessarily promise I'll be online: The context is whether or not to permit nulls as the argument to an overloaded method like this: The assumption is that the code would look difficult to read if the caller were required to pass a non-null reference here. jclouds aims to be modeled based on principles described in books such as effective java and clean code. It is actually the clean code (implied as readable code) that suggests making commands which describe their arguments. While not totally put together, there are some things listed here to help understand principles we employ: You won't find many guava methods to intentionally accept null parameters, especially if they are overloaded. Not only does the matter-of-course acceptance of null multiple the test burden of code, it also impacts readability. For example, you have to make some leap of faith to understand: "list blob names starting at null". While in jclouds, especially in old code, we have some args that have null, it is the outlier. If you want to proceed down the track of helping us get more readable, I'd start with this pull request, and make it readable. Then, you can start discussions of how allowing random fields to be null increases readability. |
|
p.s. the commentary is just that, and not a -1. Personally, I don't like In general it bothers me when we accept code that lowers our quality and/or That said, enumerating, explaining, or resolving the issues here will take Hence, I'll abstain and defer to @gaul judgement who can merge this pr when |
|
Adrian Cole » jclouds #54 SUCCESS |
|
jclouds-java-7-pull-requests #407 UNSTABLE |
| } | ||
|
|
||
| List<String> blobNames = Lists.newArrayList(); | ||
| if (!containerExists(container)) { |
There was a problem hiding this comment.
Wonky formatting. More three- and four-space indentation disagreements?
|
Can you add a small test which exercises marker? |
|
I do not have a strong opinion on the Nullable comments. We can use Optional if we want to be explicit. |
The current implementation uses a ConcurrentHashMap for storing key value pairs in the Transient blobstore. LocalAsyncBlobStore.list has to fetch all the blobs from this map and then return a subset based on the marker. Instead this change uses a ConcurrentSkipListMap which has natural ordering and sends the marker down to TransientStorageStrategy for more efficient enumeration. Fixes #1561.
|
jclouds-java-7-pull-requests #408 SUCCESS |
|
Adrian Cole » jclouds #56 SUCCESS |
The current implementation uses a ConcurrentHashMap for storing key value pairs in the Transient blobstore. LocalAsyncBlobStore.list has to fetch all the blobs from this map and then return a subset based on the marker. Instead this change uses a ConcurrentSkipListMap which has natural ordering and sends the marker down to TransientStorageStrategy for more efficient enumeration.
Fixes #1561.