Register fields types for loading type about one-nio HOT 15 CLOSED

@apangin says:

Maybe, we could discuss the original problem and come up with a better solution?

With pleasure! =)
My point is for most cases the request schema should be as exception case and do only if a dto has been changed. Now you can't use serializer if an RPC mechanism is not configured.

from one-nio.

I mean that your proposed solution covers only a small range of cases. It won't work for the common case, since there is no way to statically determine which classes will be serialized. E.g. consider the following example:

class Chat implements Serializable {
    Owner owner;
    List<Message> messages;
}

interface Owner {}

class User implements Owner { ... }

class Group implements Owner { ... }

When generating a serializer for Chat, it is not possible to find what other serializers will be used for Chat instances, because Owner is an interface with unknown set of implementations, and List is just a collection of arbitrary objects.

from one-nio.

It won't work for the common case,

I'm agree if we talk about common java-classes but in my experience serializer was used for Data Tranfer Objects (DTO). That kind of classes should be simple as much as possible because it's just a data. In my current project all dto has a structure like this:

public class Action {
    private long id;
    private EnumType type;
    private ActionParameters parameters; 
}

public class ActionParameters {
    String userName;
    String comment;
    String rejectReason;
}

Any way do you not agree that it should be no SerializerNotFoundException if in a receiver's JVM has beel loaded class with exactly the same structure as on a sender's JMV already? (For simple cases when we can statically determine which classes will be serialized)

from one-nio.

Another way to avoid a redundant SerializerNotFoundException we can try to find missed class in fields when exception is occured.

from one-nio.

I'm afraid this approach is wrong for many reasons.

• DeserializeStream is a one-way stream that maintains a deserialization context. Mark/reset functionality will not work.
• Retrying readObject recursively is unsafe and potentially leads to infinite recursion.
• Complication of readObject is not desired without proving this has no performance impact.
• Constructing GeneratedSerializer for each field is highly inefficient. This is slow and may produce many redundant unreclaimable classes.

from one-nio.

It seems to me there are many "standard" ways to solve your problem.

1. Use Repository.preload to generate required serializers beforehand. If you don't wish to list all the classes manually, you may still write a method that scans class fields and calls Repository.preload for each field type.
2. Serialize some dummy DTO once, so that all required serializers will be generated automatically.
3. Mark DTOs with some annotation. During bootstrap an application will search for the annotated beans and generate serializers for them.
4. Use Serializer.persist to embed schemas.
5. Use schema exchange mechanism like one-nio RPC or load serializers from some central repository.

Does it make sense?

from one-nio.

The ways 1-3 seem like a workaround. And that things should be in a library not in user code (hello Alex Shipilev =) )
4th way makes messages bigger this is impermissible for us (we have many little messages and 1gbe interface is full already)
5th way is the best but I think this issue can do this one better because we do not make a request to server any time when application is started only when dto has been changed.

from one-nio.

DeserializeStream is a one-way stream that maintains a deserialization context. Mark/reset functionality will not work.

I've wrote test on it. Can you break one for show why mark/reset functionality will not work ?

Retrying readObject recursively is unsafe and potentially leads to infinite recursion.

No you can't. Recursive call do only when class with didn't found uid has been found.

Complication of readObject is not desired without proving this has no performance impact.

I'll write perfomance test but here overhead is only 3 variable. Catch block has no a performance penalty.

Constructing GeneratedSerializer for each field is highly inefficient. This is slow and may produce many redundant unreclaimable classes.

Please note that all code contains in catch block. If we not try to find a class in fields we will do a request to server it's network interaction and in most cases is more heavy.
But I agree this code can be oprimized. Constructing GeneratedSerializer was just the shortest way, actualy I was needed to generate uid for class.

from one-nio.

I've wrote test on it. Can you break one

"Testing shows the presence, not the absence of bugs." (E.W. Dijkstra)
Yes, I can write a counterexample, but I'd prefer not to waste time. Hint: the context array is important.

Recursive call do only when class with didn't found uid has been found.

Right, and this process may loop in some (complicated) cases. serializer.read uses uidMap to find serializers, while Repository.get uses classMap. These two maps do not necessarily match each other, i.e. you may find a serializer via classMap, but not via uidMap thus getting SerializerNotFoundException again and again.

I'll write perfomance test but here overhead is only 3 variable. Catch block has no a performance penalty.

I would not be so sure. try/catch always adds new edges to control flow graph. Even if catch block is never executed, extra edges may prevent some JIT optimizations from happening. Furthermore, the code has noticeably grown in size, and this may also affect inlining heuristics. I say "may", because the performance impact is not obvious at all. Probably there will be no degradation, but I can't be sure until I verify this on hundreds of our highloaded microservices. Since the benefits of this change are arguable, I'd prefer to avoid such risks.

Of course, this does not mean that I will not accept pull requests at all. Contributions are welcomed with pleasure, but please note that this project is widely used in our system running on thousand production servers, so the risk of code changes should correspond to the value of the feature.

from one-nio.

The ways 1-3 seem like a workaround. And that things should be in a library not in user code

I've already explained earlier why your proposed solution works only for some particular cases. On the other hand, I believe a library should handle common cases instead. Consider an example:

class TaskResult<T> implement Serializable {
    T value;
    Throwable error;
}

An algorithm based on field introspection will produce redundant serializers for java.lang.Object and java.lang.Throwable, but SerializerNotFoundException will still happen, since the real application will always send a specific value or a specific subtype of Exception.

from one-nio.

5th way is the best but I think this issue can do this one better

So why does not it work for you? "Better" in what metrics? Schema exchange happens only once at the very beginning, and does not affect further runtime performance. We've never seen schema exchange to be a bottleneck in real applications. Have you?

from one-nio.

"Better" in what metrics?

For example start up time. Our services downloads a lot of data on start if we request server for each unknown type it's hundred excess requests on each application start up. Furthermore now if application can't deserialize a message it's shout down and requires to upgrade (yes it's pain... that's why I search a new serializer) I wanted commissioning this project step by step, not all in one release.

So why does not it work for you?

It works (maybe) but it's not too easy refactor big system to the new approach (see above) and anyway I'm still sure that to reduce count of schema-requests to minimum is the good idea. I've never told that schema-requests are not needed this is the great idea for dto with generics, interfaces, when schema has changed etc and I hope I will use one in future.

from one-nio.

Ah, I see. You haven't tried one-nio for real yet, but you think it can be improved.
How much of start-up time do you plan to save with this "optimization"? Milliseconds? Seconds? Minutes?
Would you like to measure it first?

For example, compare the time of your findSerializerInFields method (for an average class of 5 fields) to a network round-trip time within the same datacenter.
The results may look surprising (spoiler: network request will be faster).

from one-nio.

Ah, I see. You haven't tried one-nio for real yet

I have and first what I've got is SerializeNotFoundException and after that it's about hour of time to find that it haven't found serializer for UUID (That's why I added custom at the first PR) and after SerializeNotFoundException for BigInteger etc.
Yes, maybe talk about start up time is wrong. As you right told

performance impact is not obvious at all.

In the first message of this issue was good "better" metric:

For more convenient to use

But still in case when user can't use a request of schema (for some reasons) this serializer is not working without hacks like

Serialize some dummy DTO once

And I try to solve this problem. That is all.

from one-nio.

OK, I understood your point. Very sorry that I spended your time. Thank you @apangin for this discussion.

from one-nio.