Why are Storage classes Serializable?

[jboynes]

For example SignedUrIOption

This does not seem like something that would need to marshalled for transmission between processes.

[aozarov]

There are different types of Storage classes. Not all are Serializable.

• Model objects - such as BucketInfo, BlobInfo
• Request/response level options & messages - such as CopyRequest and SignUrlOption
• Service configuration - such as StorageOptions
• Functional classes - such as Storage and Blob/Bucket (to come soon - Should Blobs and Buckets be stateless or keep a reference to a StorageService ? #55).
• Blob's reader/writer channel

Functional classes are not Serialzable.

I think it is easier to justify making model objects as serializable to support a convenient way
to store them in other places if need be (e.g. memcache, datastore,...)

Having a serialize service options is also convenient to support easy suspend and resume later.
For a similar reason the blob's reader/writer are serialize (this case required some effort).
This is very important in restricted platforms that may limit the user in completing its task
in one request or process but could be used for various reasons (including easier error handling).

Last is the request/response level options and messages. I think this case has the weakest argument for having them Serialize but as all non functional are already immutable and Serializable I thought it would
make sense to maintain the same pattern and be consistent.
In addition I could think of various ways that this may be useful for higher level libraries (e.g. as a mean to replicate settings (optionally from various sources)) but if that was the only reason I would have
waited until requested.

[jboynes]

I asked because of the downsides of Serializable especially when it comes to objects that might be stored for a while.

Model Objects: I can see the use-case for storing identifiers but for not storing the entire metadata set, especially when it contains volatile fields like the ACL or update time. Relates back to #62 about separating identity and metadata.

Reader/Writer: Channel implementations are not typically Serializable - they are wrappers for I/O conduits like a file descriptor - so adding that functionality here is a bit odd. The suspend case sounds like its primarily to work around issues on some platforms (AppEngine I assume) which shouldn't be in a general purpose API but moved to an platform specific component.

Request options: These are really part of the functional interface so should be consistent with that (i.e. they do not need to be Serializable).

Service Configuration: The serialized form is not editable/readable so is not very useful. The user will already have defined this in a more readable form (json, XML, config code) so this would be redundant.

[aozarov]

Yes, I am aware.

Focus was more on convince rather than efficient (which I don't think is an issue in this case).

I think allowing an easy way to cache (or use alternate typically faster/cheaper storage) of model objects (even if part of their data is volatile) is useful but we should probably get more feedback on that.

Yes, completely agree, Channel implementation are not typically Serialize however GCS gets significant usage traffic from AE and its users are mostly using appengine-gcs-client familiar with the concept (and need the resume-able feature).

Supporting AE out-of-the-box (without any extensions) is something that I think this library should do but inn this case, instead of making the class serialize (and support transparent serialization) we could be more explicit saving-to and restoring-from a state.

I have no problems removable the Serialize from Request options. Indeed they are part of the functional interface but they do represent an immutable information (similar to data-model).

A serialize config allow easy restore/duplicate service. We could make the serializable form human readable if we see a demand for it.

More opinions about this issue would be useful. @eamonnmcmanus ?

[eamonnmcmanus]

Java Serialization is a giant pain, especially if you have to consider compatibility between the version serializing and the version deserializing. Let me tell you some JMX war stories some day when you feel like suffering. So I would be in favour of keeping its use to a minimum. I think it would better to have classes initially not be Serializable and add serializability on demand, rather than putting it there by default in case it proves useful.

Furthermore, I don't think interfaces should ever extend Serializable, so I would remove it from the ones that currently do. To my mind, Serializable is a (badly-designed) implementation flag, not a promise to clients of an interface. You could add text suggesting that implementations of the interfaces in question should be serializable, if that is appropriate and useful.

(The interfaces currently extending Serializable look to be the *RpcFactory interfaces from the spi package and the *Channel interfaces from the storage package.)

Explicit save and restore methods, perhaps using a text format, do appear to me to be a better option where appropriate than Java Serialization.

[aozarov]

OK, I flagging this issue for "Needed for Public Announcement" and hopefully we can also hear from some users.

I guess my mind set is too much influenced from the AE APIs (e.g. much simple to use the cache API when classes are serialize) and I think in that case the "pain" was well handled.
I also feel that Serializable in Java JDK itself (for various data holders, when possible) is wide-spread.

My first actions would be to take the Serialzable off the interfaces and add an explicit save/restore methods to the channels. I would like the content of the channel's state to be opaque, to make future changes easier, and therefore would favor not using plain text in this case.

[mziccard]

Personally I don't see any major issue in BlobInfo and BucketInfo extending Serializable. At the same time I don't see a huge need for them to be serializable. In #62 we are discussing about adding a class to represent an object identity. What about dropping serializable in BlobInfo and BucketInfo while having a serializable identity class?

I agree to remove Serializable from interfaces and channels while adding save and restore methods. Just to be sure: how do you expect those methods signature to be?

[aozarov]

What about dropping serializable in BlobInfo and BucketInfo while having a serializable identity class?

Then I would ask, why? In AE it was very useful to have data-models as Serializable as it allowed, for example, easy caching. BlobId would be simple enough to serialize manually and why would be
care just about it. All the ones mentioned are also immutable, simple data-models and I would not expect
any serialization problems with them, but if I am in a minority than we can remove them for now.

I totally agree that we should remove Serializable from Channels and instead provide an explicit save/restore methods (in the case of the writer adding a javadoc warning that only one thread should
be writing at a time).

[mziccard]

On save/restore methods: BlobReadChannelImpl and BlobWriteChannelImpl have package scope so the user is not meant to have direct access to them. Give than, how are users supposed to restore objects of classes BlobReadChannelImpl and BlobWriteChannelImpl if they cannot access/instantiate them?

[aozarov]

Are you saying this could not be defined in the interfaces BlobReadChannel and BlobWriteChannel?

I think the interfaces can have a "save"/captureState methods that returns another interface (implementation should guaranty to be serializable) that only has one method "restore" which returns an instance from the same type.

Somewhat little like a memento.

Another option is for them to return a more opaque object and having the "restore" by overloading the writer, reader methods to gets the captured-sate.

I have a slight preference to the first option.

[mziccard]

Either I am tired or dumb. Let's say I am tired:)
I was thinking of a more classical memento where it's duty of the Originator to restore the state. That's brilliant, I'll do that.

[aozarov]

@mziccard Anything else that is needed to be done for this issue?

[mziccard]

The only point that may be still open is whether ServiceRpcFactory should extend Serializable or not.

[aozarov]

Do you see a way to implement suspend and resume in a separate process without it?

[mziccard]

@aozarov We could once again add save and restore methods to the interface, right?

[aozarov]

Oh, sure. but the main point is that we are still going to require from the implementation of the factory
to be able to "serialize/deserialize" itself. I think that is a reasonable expectation from a factory (as oppose to the object it creates).

I totally agree that serialize channels is unexpected and therefore was all for changing them to provide
explicit save/restore.

I don't think it is that terrible for factories however I don't mind to change it to be more explicit as well.
Another option would be to require the factory implementation to be public and have an empty constructor (so we can just save the class name and recreate it as needed).

I am actually experimenting with a fix for #54 where we would still have a notion of a service factory but would that setting would be done on the options and therefore allow the options to create a service. And, as we already provide a way to load the factory using a ServiceLoader which requires
a publicly accessible empty constructor maybe we should pick that (and I can bundle it with my change)?

What do you think?

[mziccard]

@aozarov I agree that adding save/restore methods still forces implementations to be able to serialize. However, if it is the drawbacks of Serializable we are worried about, using such methods allows to avoid them. And implementations can still make save/restore throw UnsupportedOperationException.

That said, I think that requiring factory implementations to be public and have an empty constructor makes perfectly sense (more than having save/restore methods) and if you could incorporate that in your changes for #54 that would a perfect way of closing this issue as well. I like this solution and I'll be happy to review the changes.

[mziccard]

Closed with #261 and #296.
If you feel that something is still missing to address this issue feel free to reopen.