[infinispan-dev] [ISPN-78] Feedback needed

[Galder Zamarreño]

Hey Olaf,

First of all, thanks for work put so far on this! 

Pity you could not make it last week to Berlin Expert Days. I would have been great to sit down and go through this code together :| - hope you're doing better health wise :)

Here are my thoughts so far:

- First of all, since you have made several commits, it'd be nice for the rest of audience to highlight what the changes are at a high level so that people can focus on the code immediately.

- I actually like some aspects of writeObject(key, InputStream) API cos it does the reading for the user and that can be done directly in the desired chunk size to pass it around and this is done internally, so the user has to write less code. On the other side, if we had a 'OutputStream writeToKey(K key);' the user has the freedom to decide how to read from an external stream and write to the outpustream but then we'd probably have to chunk it again to send it around the cluster cos the chunks in which the outputstream is written are not necessarily the chunks we'll write to the cluster. The only major problem I see with writeObject(key, InputStream) is if the reading the input stream blocks. So, imagine that the input stream is slow, i.e. a remote cloud provider. Then the client code is blocked and can't do anything and this is not good, so if we go with writeObject(key, InputStream), it needs to throw InterruptedException. On the other hand, the suggested code in http://community.jboss.org/wiki/LargeObjectSupport allows for the client to tweak how the input stream is read to avoid blocking, i.e. have some kind of NIO way of reading the stream.

- Rather than modifying PutKeyValueCommand, it might be better to subclass it and add the large object logic there? i.e. PutLargeKeyValueCommand - I'd also suggest adding a new build* method in the command factory...etc. This keeps the code more fine grained.

- I don't think it's a good idea to have KeyGenerator<K> generalized. It's just complicates the code. Simply have an Object as key and it would simplify the code. Besides, you have hardcoded Chunk to a String type of chunk key and that's probably not right cos it depends on the key generator.

- Internally we don't use K of the Cache interface, we simply stick to Object to simplify the code. Externally, in the Cache interface we do keep generics but we don't use them internally. So you can remove K from LargeObjectMetadata and Chunks.

Cheers,

On Apr 10, 2011, at 10:07 PM, Olaf Bergner wrote:

> I have the skeleton of an implementation of ISPN-78 - Large Object 
> Support - at https://github.com/obergner/infinispan/tree/t_ispn78. 
> Before going forward I could need some feedback on whether my approach 
> makes sense at all, what alternatives there are, where things might be 
> improved or modified to adhere to INFINISPAN's standards and so forth. 
> Any hint is highly appreciated.
> 
> Keep in mind that so far I have completely ignored the issue of 
> supporting transactions when reading and writing large objects. I would 
> prefer to have a working core implementation before tackling the more 
> complicated aspects.
> 
> Cheers,
> Olaf
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev

--
Galder Zamarreño
Sr. Software Engineer
Infinispan, JBoss Cache