You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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.
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 ?
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.
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.
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?
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).
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?
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.
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.
Activity
aozarov commentedon Jun 10, 2015
There are different types of Storage classes. Not all are Serializable.
BucketInfo,BlobInfoCopyRequestand SignUrlOptionStorageandBlob/Bucket(to come soon - Should Blobs and Buckets be stateless or keep a reference to a StorageService ? #55).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 commentedon Jun 11, 2015
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 commentedon Jun 11, 2015
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 commentedon Jun 11, 2015
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
*RpcFactoryinterfaces from thespipackage and the*Channelinterfaces from thestoragepackage.)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 commentedon Jun 11, 2015
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 commentedon Oct 11, 2015
Personally I don't see any major issue in
BlobInfoandBucketInfoextendingSerializable. 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 inBlobInfoandBucketInfowhile having a serializable identity class?I agree to remove Serializable from interfaces and channels while adding
saveandrestoremethods. Just to be sure: how do you expect those methods signature to be?aozarov commentedon Oct 12, 2015
Then I would ask, why? In AE it was very useful to have data-models as Serializable as it allowed, for example, easy caching.
BlobIdwould be simple enough to serialize manually and why would becare 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 commentedon Oct 14, 2015
On save/restore methods:
BlobReadChannelImplandBlobWriteChannelImplhave package scope so the user is not meant to have direct access to them. Give than, how are users supposed to restore objects of classesBlobReadChannelImplandBlobWriteChannelImplif they cannot access/instantiate them?aozarov commentedon Oct 14, 2015
Are you saying this could not be defined in the interfaces
BlobReadChannelandBlobWriteChannel?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,readermethods to gets the captured-sate.I have a slight preference to the first option.
mziccard commentedon Oct 14, 2015
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 commentedon Oct 23, 2015
@mziccard Anything else that is needed to be done for this issue?
mziccard commentedon Oct 23, 2015
The only point that may be still open is whether
ServiceRpcFactoryshould extendSerializableor not.13 remaining items
chore(main): release 0.5.5 (#104)
chore(main): release 1.1.3 (#123)
chore(main): release 0.3.8 (googleapis#109)
chore(java): add a note in README for migrated split repos (googleapi…
deps: update core deps (#109)
chore: release 1.101.0 (#106)
Retry GCE environment check for Application Default Credentials (#110)
deps: update core deps (googleapis#109)
chore: release 1.101.0 (googleapis#106)
Retry GCE environment check for Application Default Credentials (#110)
chore(deps): update dependency com.google.cloud.samples:shared-config…
chore: remove duplicate dependencies (#109)
chore: remove duplicate dependencies (#109)
build(deps): update dependency org.apache.maven.plugins:maven-javadoc…
build(deps): update dependency org.apache.maven.plugins:maven-javadoc…