Nikita, It sounds like the test should be changed, no? In case I'm missing something, can you please give more details about the scenario which requires deserialization? Generally, this sounds weird - in cases when we can get advantage of binary format and avoid deserialization, we definitely should not deserialize.
-Val On Tue, Sep 19, 2017 at 6:17 AM, Nikita Amelchev <[email protected]> wrote: > I have some problem when we don't deserialize Externalizable. Some messages > require deserializing in GridCacheIoManager.message0(). For example, tests > like testResponseMessageOnUnmarshallingFailed where readExternal throws an > exception. A message containing Externalizable is deserialized and > processed as a failed message. If we do not deserialize here, we won't > process this message as failed. What way to resolve it? I see we can try to > deserialize after a check on Externalizable in a finishUnmarshall method, > but it looks bad. What are your thoughts? > > 2017-09-07 12:57 GMT+03:00 Nikita Amelchev <[email protected]>: > > > I also agree that we should try to use Externalizable without > > deserialization on servers. I will do it in a way suggested in the > review. > > The Externalizable will marshal through type GridBinaryMarshaller.OBJ, in > > addition, I’ll add a flag in BinaryConfiguration which will be used to > turn > > on the old way with OptimizedMarshaller for compatibility with the > current > > data format. > > > > 2017-09-06 4:21 GMT+03:00 Dmitriy Setrakyan <[email protected]>: > > > >> Vova, I agree. Let's stay loyal to our binary serialization protocol. > >> > >> Do you know if we will be loosing any functionality available in > >> Externalizable, but not present in our binary protocol? > >> > >> D. > >> > >> On Mon, Sep 4, 2017 at 11:38 PM, Vladimir Ozerov <[email protected]> > >> wrote: > >> > >> > Folks, > >> > > >> > Let's discuss how this should be handled properly. I proposed to use > the > >> > same format as regular binary object, with all data being written in > >> "raw" > >> > form. This would give us one critical advantage - we will be able to > >> work > >> > with such objects without deserialization on the server. Hence, no > >> classes > >> > will be needed on the server side. Current implementation (see PR in > the > >> > ticket) defines separate format which require deserialization, I am > not > >> OK > >> > with it. > >> > > >> > Thoughts? > >> > > >> > On Wed, Aug 23, 2017 at 6:11 PM, Nikita Amelchev < > [email protected]> > >> > wrote: > >> > > >> > > Hello, Igniters! > >> > > > >> > > I've developed Externalizable interface support using > BinaryMarshaller > >> > [1]. > >> > > > >> > > I have a misunderstanding with defining BinaryWriteMode in > >> > > BinaryUtils.mode(cls): there is AffinityKey class which implements > >> > > Externalizable and registered with ReflectiveSerialize, > >> BinaryMarshaller > >> > > defines it as BinaryWriteMode.OBJ and uses reflection according to > >> > current > >> > > logic. I want to say that AffinityKey must be defined as > >> > > BinaryWriteMode.OBJ although the class implements the Externalizable > >> > > interface. > >> > > I have to add a new one more parameter in BinaryUtils.mode(cls) to > >> define > >> > > BinaryWriteMode in a proper way. > >> > > Could you please review and comment my solution [2]? > >> > > > >> > > BTW, I have benchmarked my solution by GridMarshallerPerformanceTest > >> and > >> > it > >> > > becomes faster up to 2 times (GridMarshaller).My JMH tests show that > >> > > marshal faster up to 50% and unmarshal faster up to 100% on an > >> > > Externalizable object. > >> > > > >> > > Also, I've filed an issue for Serializable interface support using > >> > > BinaryMarshaller [3] as it has been described earlier. > >> > > > >> > > [1] https://issues.apache.org/jira/browse/IGNITE-2894 > >> > > [2] https://reviews.ignite.apache.org/ignite/review/IGNT-CR-278 > >> > > [3] https://issues.apache.org/jira/browse/IGNITE-6172 > >> > > > >> > > 2017-08-21 20:43 GMT+03:00 Valentin Kulichenko < > >> > > [email protected]>: > >> > > > >> > > > Nikita, > >> > > > > >> > > > I think anything binary related should have higher priority than > >> > > > Externalizable. I.e. if user explicitly implemented Binarylizable > or > >> > > > provided a BinarySerializer, then BinaryMarshaller should of > course > >> use > >> > > > that and ignore Externalizable. > >> > > > > >> > > > -Val > >> > > > > >> > > > On Mon, Aug 21, 2017 at 9:29 AM, Nikita Amelchev < > >> [email protected] > >> > > > >> > > > wrote: > >> > > > > >> > > > > Hello, Igniters. > >> > > > > > >> > > > > I am developing Externalizable interface support by > >> BinaryMarshaller > >> > > > > through new type constant. BinaryMarshaller allows using > >> > > BinarySerializer > >> > > > > to manage serialization. I need to define BinaryWriteMode in the > >> > > > > BinaryClassDescriptor constructor. In case of the Binarylizable > >> > > > interface - > >> > > > > serializer is ignored and BinaryWriteMode is BINARY. Can I do > the > >> > same > >> > > > with > >> > > > > the Externalizable interface? > >> > > > > > >> > > > > In this case, I have issues with AffinityKey: some tests have > >> failed > >> > > > > because of they except serialization logic of defined the > >> serializer > >> > > > > instead of Externalizable logic. What is the priority between > >> > > predefined > >> > > > > BinarySerializer for class and implementation of Externalizable > >> > > > interface? > >> > > > > > >> > > > > 2017-08-01 13:09 GMT+03:00 Vladimir Ozerov < > [email protected] > >> >: > >> > > > > > >> > > > > > Valya, > >> > > > > > It makes sense to have both Externalizable and Binarylizable, > as > >> > user > >> > > > > might > >> > > > > > want to serialize object for different systems. E.g. > deserialize > >> > > binary > >> > > > > > stream from Kafka in Externalizable mode, and then put it to > >> Ignite > >> > > > with > >> > > > > > Binarylizable to allow for field access without > deserialization. > >> > > > > > > >> > > > > > Nikita, > >> > > > > > I think that Externalizable should be written in the same way > >> as we > >> > > > write > >> > > > > > fields in "raw" mode. So may be it will be enough to simply > >> > implement > >> > > > our > >> > > > > > own ObjectOutput interface on top of existing > >> BinaryWriterExImpl. > >> > > Makes > >> > > > > > sense? > >> > > > > > > >> > > > > > Vladimir. > >> > > > > > > >> > > > > > On Thu, Jun 22, 2017 at 1:30 AM, Valentin Kulichenko < > >> > > > > > [email protected]> wrote: > >> > > > > > > >> > > > > > > Hi Nikita, > >> > > > > > > > >> > > > > > > 1. Makes sense to me. > >> > > > > > > > >> > > > > > > 2. Externalizable object should not be written as binary > with > >> > flag > >> > > > 103, > >> > > > > > it > >> > > > > > > should be written in the same way it's written now. I don't > >> see > >> > any > >> > > > > > reason > >> > > > > > > to change the protocol. Purpose of this task it to move the > >> logic > >> > > to > >> > > > > > binary > >> > > > > > > marshaller instead of depending on optimized marshaller, and > >> also > >> > > > fully > >> > > > > > > support handles for these objects and objects included in > >> them. > >> > > > > Currently > >> > > > > > > binary marshaller and optimized marshaller use different set > >> of > >> > > > > handles - > >> > > > > > > this is the main downside of current implementation. > >> > > > > > > > >> > > > > > > 3. I think this order is correct, but does it even make > sense > >> to > >> > > > > > implement > >> > > > > > > both Binarylizable and Externalizable? > >> > > > > > > > >> > > > > > > -Val > >> > > > > > > > >> > > > > > > On Mon, Jun 19, 2017 at 8:58 AM, Nikita Amelchev < > >> > > > [email protected] > >> > > > > > > >> > > > > > > wrote: > >> > > > > > > > >> > > > > > > > Hello everebody. > >> > > > > > > > > >> > > > > > > > I would like to clarify about some moments in marshaller > >> about > >> > > > custom > >> > > > > > > > serialization. > >> > > > > > > > > >> > > > > > > > 1. I suggest to divide the issue into two tasks: support > the > >> > > > > > > Externalizable > >> > > > > > > > and support the Serializable. The second task is to do as > a > >> > > > separate > >> > > > > > > issue. > >> > > > > > > > > >> > > > > > > > 2. In case the Optimized marshaller when object is the > >> > > > Extenalizable > >> > > > > > > > BinaryUtils.unmarshal() return deserialize value. But if > we > >> > will > >> > > > not > >> > > > > > use > >> > > > > > > > Optimized marshaller and write the Extenalizable as the > >> > > Object(103) > >> > > > > it > >> > > > > > > > return the BinaryObjectExImpl. It break > >> > > testBuilderExternalizable. > >> > > > > (If > >> > > > > > we > >> > > > > > > > replace Externalizable to Binarilylizable it also dont > >> work). > >> > > Fix - > >> > > > > > check > >> > > > > > > > that object is the Extenalizable and deserialize > >> > > > > > > > manual(BinaryUtils.java:1833 in PR). We will use this fix > or > >> > > return > >> > > > > > > > BinaryObjectExImpl? > >> > > > > > > > > >> > > > > > > > 3. What are priority if was implemented several > interfaces: > >> > > > > > Binarylizable > >> > > > > > > > -> Externalizable -> Serializable ? > >> > > > > > > > > >> > > > > > > > Also can you pre review this issue? > >> > > > > > > > PR: https://github.com/apache/ignite/pull/2160 > >> > > > > > > > > >> > > > > > > > 2017-04-18 17:41 GMT+03:00 Valentin Kulichenko < > >> > > > > > > > [email protected]>: > >> > > > > > > > > >> > > > > > > > > Nikita, > >> > > > > > > > > > >> > > > > > > > > For Externalizable option 1 is the correct one. > >> > Externalizable > >> > > > > > objects > >> > > > > > > > > should not be treated as binary objects. > >> > > > > > > > > > >> > > > > > > > > For read/writeObject, you indeed have to extend > >> > > > ObjectOutputStream. > >> > > > > > > > > writeObject() is final because you should extend > >> > > > > > writeObjectOverride() > >> > > > > > > > > instead. Take a look at ObjectOutputStream's JavaDoc and > >> on > >> > how > >> > > > > this > >> > > > > > is > >> > > > > > > > > done in OptimizedObjectOutputStream. Note that ideally > we > >> > need > >> > > to > >> > > > > > > > implement > >> > > > > > > > > everything that is included in Java serialization spec, > >> > > including > >> > > > > > some > >> > > > > > > > > non-trivial stuff like PutField. I would check if it's > >> > possible > >> > > > to > >> > > > > > > > somehow > >> > > > > > > > > reuse the code that already exists in optimized > >> marshaller as > >> > > > much > >> > > > > as > >> > > > > > > > > possible. > >> > > > > > > > > > >> > > > > > > > > -Val > >> > > > > > > > > > >> > > > > > > > > On Tue, Apr 18, 2017 at 1:36 PM, Nikita Amelchev < > >> > > > > > [email protected] > >> > > > > > > > > >> > > > > > > > > wrote: > >> > > > > > > > > > >> > > > > > > > > > I see two ways to support the Externalizable in the > BM: > >> > > > > > > > > > 1. Add a new type constant to the GridBinaryMarshaller > >> > class > >> > > > etc > >> > > > > > and > >> > > > > > > > > > read/writeExternal in the BinaryClassDescriptor. > >> > > > > > > > > > 2. Make read/writeExternal through the BINARY type > >> without > >> > > > > updating > >> > > > > > > > > > metadata. > >> > > > > > > > > > I don't know how to make a support read/writeObject of > >> the > >> > > > > > > Serializable > >> > > > > > > > > > without delegating to the OM. Because read/writeObject > >> > > methods > >> > > > > need > >> > > > > > > the > >> > > > > > > > > > Objectoutputstream class argument. One way is to > >> delegate > >> > it > >> > > to > >> > > > > the > >> > > > > > > > > > OptimizedObjectOutputStream. Second way is to extend > the > >> > > > > > > > > Objectoutputstream > >> > > > > > > > > > in the BinaryWriterExImpl. But it is wrong way because > >> the > >> > > > > > > writeObject > >> > > > > > > > is > >> > > > > > > > > > final. > >> > > > > > > > > > > >> > > > > > > > > > 2017-01-19 20:46 GMT+03:00 Valentin Kulichenko < > >> > > > > > > > > > [email protected]>: > >> > > > > > > > > > > >> > > > > > > > > > > Nikita, > >> > > > > > > > > > > > >> > > > > > > > > > > In my view we just need to support Externalizable > and > >> > > > > > > > > > > writeObject/readObject in BinaryMarshaller and get > >> rid of > >> > > > > > > delegation > >> > > > > > > > to > >> > > > > > > > > > > optimized marshaller. Once such classes also go > >> through > >> > > > > > > > > BinaryMarshaller > >> > > > > > > > > > > streams, they will be aware of binary configuration > >> and > >> > > will > >> > > > > > share > >> > > > > > > > the > >> > > > > > > > > > same > >> > > > > > > > > > > set of handles as well. This should take care of all > >> the > >> > > > issues > >> > > > > > we > >> > > > > > > > have > >> > > > > > > > > > > here. > >> > > > > > > > > > > > >> > > > > > > > > > > -Val > >> > > > > > > > > > > > >> > > > > > > > > > > On Thu, Jan 19, 2017 at 7:26 AM, Nikita Amelchev < > >> > > > > > > > [email protected] > >> > > > > > > > > > > >> > > > > > > > > > > wrote: > >> > > > > > > > > > > > >> > > > > > > > > > > > I have some questions about single Marshaller. > >> > > > > > > > > > > > It seems not easy to merge OptimizedMarshaller > with > >> > > > > > > > BinaryMarshaller > >> > > > > > > > > > and > >> > > > > > > > > > > is > >> > > > > > > > > > > > there any sense in it? > >> > > > > > > > > > > > When Binary object inside Externalizable > serialized > >> > with > >> > > > > > > optimized > >> > > > > > > > it > >> > > > > > > > > > > > losing all benefits. > >> > > > > > > > > > > > Will OptimizedMarshaller be supported at 2.0 > >> version? > >> > Or > >> > > to > >> > > > > > merge > >> > > > > > > > > they > >> > > > > > > > > > is > >> > > > > > > > > > > > better? > >> > > > > > > > > > > > What do you think about it? > >> > > > > > > > > > > > > >> > > > > > > > > > > > In addition, Vladimir Ozerov, I would like to hear > >> your > >> > > > > > opinion. > >> > > > > > > > > > > > > >> > > > > > > > > > > > 2017-01-17 23:32 GMT+03:00 Denis Magda < > >> > > [email protected] > >> > > > >: > >> > > > > > > > > > > > > >> > > > > > > > > > > > > Someone else added you to the contributors list > in > >> > > JIRA. > >> > > > > This > >> > > > > > > is > >> > > > > > > > > why > >> > > > > > > > > > I > >> > > > > > > > > > > > > couldn’t add you for the second time. Ignite > >> > > committers, > >> > > > > > please > >> > > > > > > > > reply > >> > > > > > > > > > > on > >> > > > > > > > > > > > > the dev list if you add someone to the list. > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > Nikita, yes, this ticket is still relevant. Go > >> ahead > >> > > and > >> > > > > > assign > >> > > > > > > > it > >> > > > > > > > > on > >> > > > > > > > > > > > > yourself. > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > Also please you may want to help with > approaching > >> 2.0 > >> > > > > release > >> > > > > > > and > >> > > > > > > > > > take > >> > > > > > > > > > > > > care of one of the sub-tasks that must be > >> included in > >> > > > 2.0: > >> > > > > > > > > > > > > https://issues.apache.org/ > jira/browse/IGNITE-4547 > >> < > >> > > > > > > > > > > > > https://issues.apache.org/ > jira/browse/IGNITE-4547 > >> > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > — > >> > > > > > > > > > > > > Denis > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > On Jan 15, 2017, at 9:02 PM, Nikita Amelchev < > >> > > > > > > > > [email protected] > >> > > > > > > > > > > > >> > > > > > > > > > > > > wrote: > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > This issue was created long ago. Is still > >> relevant? > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > JIRA account: > >> > > > > > > > > > > > > > Username: NSAmelchev > >> > > > > > > > > > > > > > Full Name: Amelchev Nikita > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > 2017-01-14 1:52 GMT+03:00 Denis Magda < > >> > > > [email protected] > >> > > > > >: > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > >> Hi Nikita, > >> > > > > > > > > > > > > >> > >> > > > > > > > > > > > > >> I can’t find provided account in Ignite JIRA > >> > > > > > > > > > > > > >> https://issues.apache.org/jira/browse/IGNITE > < > >> > > > > > > > > > > > > https://issues.apache.org/ > >> > > > > > > > > > > > > >> jira/browse/IGNITE> > >> > > > > > > > > > > > > >> > >> > > > > > > > > > > > > >> Please create an account there and share with > >> me. > >> > > > > > > > > > > > > >> > >> > > > > > > > > > > > > >> This information might be useful for you as > >> well. > >> > > > > > > > > > > > > >> > >> > > > > > > > > > > > > >> Subscribe to both dev and user lists: > >> > > > > > > > > > > > > >> https://ignite.apache.org/ > >> > > > > community/resources.html#mail- > >> > > > > > > lists > >> > > > > > > > > > > > > >> > >> > > > > > > > > > > > > >> Get familiar with Ignite development process > >> > > described > >> > > > > > here: > >> > > > > > > > > > > > > >> https://cwiki.apache.org/ > >> > confluence/display/IGNITE/ > >> > > > > > > > > > > > Development+Process > >> > > > > > > > > > > > > >> > >> > > > > > > > > > > > > >> Instructions on how to contribute can be > found > >> > here: > >> > > > > > > > > > > > > >> https://cwiki.apache.org/ > >> > > > confluence/display/IGNITE/How+ > >> > > > > > > > > > > to+Contribute > >> > > > > > > > > > > > > >> > >> > > > > > > > > > > > > >> Project setup in Intellij IDEAL > >> > > > > > > > > > > > > >> https://cwiki.apache.org/ > >> > confluence/display/IGNITE/ > >> > > > > > > > > Project+Setup > >> > > > > > > > > > > > > >> > >> > > > > > > > > > > > > >> Regards, > >> > > > > > > > > > > > > >> Denis > >> > > > > > > > > > > > > >> > >> > > > > > > > > > > > > >>> On Jan 13, 2017, at 1:37 AM, Nikita > Amelchev < > >> > > > > > > > > > [email protected] > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> wrote: > >> > > > > > > > > > > > > >>> > >> > > > > > > > > > > > > >>> Hello everyone. > >> > > > > > > > > > > > > >>> > >> > > > > > > > > > > > > >>> I'd like to take IGNITE-2894. Can you assign > >> to > >> > me? > >> > > > > > > > > > > > > >>> > >> > > > > > > > > > > > > >>> Username: NSAmelchev > >> > > > > > > > > > > > > >>> > >> > > > > > > > > > > > > >>> -- > >> > > > > > > > > > > > > >>> Best wishes, > >> > > > > > > > > > > > > >>> Amelchev Nikita > >> > > > > > > > > > > > > >> > >> > > > > > > > > > > > > >> > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > -- > >> > > > > > > > > > > > > > Best wishes, > >> > > > > > > > > > > > > > Amelchev Nikita > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > -- > >> > > > > > > > > > > > Best wishes, > >> > > > > > > > > > > > Amelchev Nikita > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > -- > >> > > > > > > > > > Best wishes, > >> > > > > > > > > > Amelchev Nikita > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > -- > >> > > > > > > > Best wishes, > >> > > > > > > > Amelchev Nikita > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > > -- > >> > > > > Best wishes, > >> > > > > Amelchev Nikita > >> > > > > > >> > > > > >> > > > >> > > > >> > > > >> > > -- > >> > > Best wishes, > >> > > Amelchev Nikita > >> > > > >> > > >> > > > > > > > > -- > > Best wishes, > > Amelchev Nikita > > > > > > -- > Best wishes, > Amelchev Nikita >
