Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support to use application-fury in resteasy #26

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

zhfeng
Copy link
Contributor

@zhfeng zhfeng commented Nov 13, 2024

This will support to use application/fury as a mediatype and de(serialize) objects over the HTTP connections.

see testFuryBar() method.

@zhfeng zhfeng requested a review from a team as a code owner November 13, 2024 07:50
@zhfeng
Copy link
Contributor Author

zhfeng commented Nov 13, 2024

@chaokunyang can you take a look at the native test failure. It seems that Bar and ThirdPartyBar has the same record

int f1, string f2

In JVM mode, it is deserialized into Bar but in native mode, it is into ThirdPartyBar.

2024-11-13 07:54:17,672 ERROR [org.jbo.res.res.i18n] (executor-thread-1) RESTEASY002005: Failed executing GET /fury/test: org.jboss.resteasy.spi.InternalServerErrorException: RESTEASY003020: Bad arguments passed to public io.quarkiverse.fury.it.Bar io.quarkiverse.fury.it.FuryResources.testBar(io.quarkiverse.fury.it.Bar)  ( io.quarkiverse.fury.it.ThirdPartyBar ThirdPartyBar[f1=1, f2=hello bar] )

@zhfeng
Copy link
Contributor Author

zhfeng commented Nov 13, 2024

@chaokunyang if there two class have the same fields, like

class A {
    int a;
    String b;
}

class B {
    int a;
    String b;
}

It should be serialze into the same bytes, right? during deserilzing, how `fury` can detect which class type should be used?

@chaokunyang
Copy link
Contributor

Fury generated codec is cached into a global map, maybe they have a conflict. When creating ThreadSafeFury, we could a different name by FuryBuilder#withName to avoid cache conflict.

@chaokunyang
Copy link
Contributor

Maybe We could Set name to timestamp for tests

@zhfeng
Copy link
Contributor Author

zhfeng commented Nov 13, 2024

Hmm, another found that Bar and ThirdPartyBar looks like use the very similar custom Serializer. BarSerializer and ThridPartyBarSerializer. Is that a problem?

public class BarSerializer extends Serializer<Bar> {
    public BarSerializer(Fury fury, Class<Bar> type) {
        super(fury, type);
    }

    @Override
    public void write(MemoryBuffer buffer, Bar value) {
        buffer.writeVarInt32(value.f1());
        fury.writeJavaString(buffer, value.f2());
    }

    @Override
    public Bar read(MemoryBuffer buffer) {
        return new Bar(buffer.readVarInt32(), fury.readJavaString(buffer));
    }
}

@Consumes({ "application/fury", "application/*+fury" })
@Produces({ "application/fury", "application/*+fury" })
public class ClassicFurySerializer implements MessageBodyReader<Object>, MessageBodyWriter<Object> {
private BaseFury fury;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add @Inject here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, it shoudl work. Let me try.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it does not work. Because resteasy will init the providers at static_init time where the bean container is not running. We have to retrieve BaseFury at runtime. see

@chaokunyang
Copy link
Contributor

Hmm, another found that Bar and ThirdPartyBar looks like use the very similar custom Serializer. BarSerializer and ThridPartyBarSerializer. Is that a problem?

They are all caused by cache conflict, I fixed it in #27

@zhfeng
Copy link
Contributor Author

zhfeng commented Nov 13, 2024

OK, I will check with your fix. Thanks a lot!

@zhfeng
Copy link
Contributor Author

zhfeng commented Nov 13, 2024

Sorry @chaokunyang - your fix looks not working!

public void testFuryBar() {
Bar bar = new Bar(1, "hello bar");
Fury fury = Fury.builder().requireClassRegistration(true).withName("Fury" + System.nanoTime()).build();
fury.register(Bar.class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't write code like this, the serialization and deserialization should have same class registration order. If you register Bar here, it allocate an ID x for Bar, but the server allocate id x for ThirdPartyBar. This is why it fails. You could update like this:

@FurySerialization(classId=200)
class Bar {
...
}
fury.register(Bar.class, 200);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's the root cause. Thanks a lot @chaokunyang ! Should we need a NOTE in the documentation or refer to the Apache Fury docs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, would you like submit a pr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean a PR for Fury?

@zhfeng zhfeng merged commit 8267fce into quarkiverse:main Nov 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants