Skip to content

Why are Storage classes Serializable? #109

Closed

Description

@jboynes

For example SignedUrIOption

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

Activity

  1. aozarov commented on Jun 10, 2015

    @aozarov
    Contributor

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

    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.

  2. jboynes commented on Jun 11, 2015

    @jboynes
    Author

    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.

  3. aozarov commented on Jun 11, 2015

    @aozarov
    Contributor

    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 ?

  4. eamonnmcmanus commented on Jun 11, 2015

    @eamonnmcmanus
    Contributor

    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.

  5. aozarov commented on Jun 11, 2015

    @aozarov
    Contributor

    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.

  6. added this to the milestone on Jun 11, 2015
  7. added
    type: questionRequest for information or clarification. Not an issue.
    on Oct 10, 2015
  8. mziccard commented on Oct 11, 2015

    @mziccard
    Contributor

    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?

  9. aozarov commented on Oct 12, 2015

    @aozarov
    Contributor

    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).

  10. mziccard commented on Oct 14, 2015

    @mziccard
    Contributor

    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?

  11. aozarov commented on Oct 14, 2015

    @aozarov
    Contributor

    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.

  12. mziccard commented on Oct 14, 2015

    @mziccard
    Contributor

    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.

  13. aozarov commented on Oct 23, 2015

    @aozarov
    Contributor

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

  14. mziccard commented on Oct 23, 2015

    @mziccard
    Contributor

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

  15. 13 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

api: storageIssues related to the Cloud Storage API.type: questionRequest for information or clarification. Not an issue.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

    Sponsor
    SponsoredKunjungi sekarang
    Promo