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

[Java] FuryBuilder supports registered classes #1459

Open
LiangliangSui opened this issue Apr 4, 2024 · 8 comments
Open

[Java] FuryBuilder supports registered classes #1459

LiangliangSui opened this issue Apr 4, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@LiangliangSui
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Currently, classes can only be registered through the Fury#register interface. Could we build Fury and register the class in one go?

Some users have reported that classes cannot be registered through FuryBuilder. #1458

Describe the solution you'd like

Could we support chained(In the process of building Fury) class registration in FuryBuilder?

// e.g. 
Fury fury = Fury.builder().withLanguage(Language.JAVA)
        .requireClassRegistration(true)
        // --------- Chain execution registration.
        .register(Foo.class)
        .register(Foo1.class)
        .build();

Additional context

N / A

@LiangliangSui LiangliangSui added the enhancement New feature or request label Apr 4, 2024
@chaokunyang
Copy link
Collaborator

chaokunyang commented Apr 4, 2024

Looks good to me, we can add API for register class by order, register class by id, and register serializer by class.

@chaokunyang
Copy link
Collaborator

chaokunyang commented Apr 4, 2024

One concern is that BaseFury already supports register classes/serializers, do we still need to add this API to FuryBuilder?

@LiangliangSui
Copy link
Contributor Author

One concern is that BaseFury already supports register classes/serializers, do we still need to add this API to FuryBuilder?

We can extract all register(...) methods in BaseFury into an interface, for example named ClassRegister, and then let BaseFury extends ClassRegister, and FuryBuilder can also implements ClassRegister. registerSerializer(...) is similar, we can extract an interface named SerializerRegister.

WDYT? @chaokunyang

@chaokunyang
Copy link
Collaborator

I perfer not. Fury is not a ClassRegister, there is no such a straightforward is-a relationship between these two classes. And it's not that intuitive for a builder class to implement some interface.

@LiangliangSui
Copy link
Contributor Author

I agree with you, Fury and ClassRegister have a has-a relationship.

If we don't implement some interfaces in FuryBuilder, then we can only repeatedly define the registerXXX function, or do you have some other good suggestions?

@chaokunyang
Copy link
Collaborator

chaokunyang commented Apr 7, 2024

I still don't know why users can't invoke BaseFury to register a class/serializer? If we need a new inferface for this registration. It looks a little too heavy to me. The basic principle is try to avoid unnecessary abstraction

@LiangliangSui
Copy link
Contributor Author

It is also possible to use BaseFury to register classes and Serializers, but these steps need to be done after Fury build. It is best to register during the FuryBuilder build process, but currently, this will cause the overall structure to be very heavy (in order to achieve this function will lead to architectural redundancy).

Currently, only one user has made this suggestion. We can also observe whether other users make this suggestion in the future. If so, we will consider adjusting the structure to implement this function.

We can keep this issue for continuous observation to see if there are any similar suggestions.

@chaokunyang
Copy link
Collaborator

Ok, we can keep the issue open. If the demand for registering classes on FuryBuilder continues grow, we can support it later. And all similar methods such as setClassChecker/setSerializerFactory have similar issues. We should take those API into consideration too.

I believe we will receive more feedbacks when we made several releases under ASF.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants