dant3 commented on code in PR #3563: URL: https://github.com/apache/ignite-3/pull/3563#discussion_r1559288047
########## modules/eventlog/src/test/java/org/apache/ignite/internal/eventlog/ser/RegistryBackedEventSerializerTest.java: ########## @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.eventlog.ser; + +import static org.hamcrest.MatcherAssert.assertThat; +import static uk.co.datumedge.hamcrest.json.SameJSONAs.sameJSONAs; + +import java.util.Map; +import java.util.stream.Stream; +import org.apache.ignite.internal.eventlog.api.Event; +import org.apache.ignite.internal.eventlog.api.IgniteEvents; +import org.apache.ignite.internal.eventlog.event.EventImpl; +import org.apache.ignite.internal.eventlog.event.EventUser; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +class RegistryBackedEventSerializerTest { + private static Stream<Arguments> events() { + return Stream.of( + Arguments.of( + new CustomEventBuilder() + .productVersion("3.0.0") + .timestamp(1234567890) + .user(EventUser.of("test_user", "test_provider")) + .message(new Message(1, "test")) + .build(), + "{\"type\":\"CUSTOM\"," + + "\"timestamp\":1234567890," + + "\"productVersion\":\"3.0.0\"," + + "\"user\":{\"username\":\"test_user\",\"authenticationProvider\":\"test_provider\"}," + + "\"message\":{\"version\":1,\"body\":\"test\"}," + + "\"fields\":{\"hasMessage\":true}" + + "}" + ), + Arguments.of( + IgniteEvents.USER_AUTHENTICATED.builder() + .productVersion("3.0.0") + .timestamp(1234567890) + .user(EventUser.of("test_user", "test_provider")) + .fields(Map.of("ip", "127.0.0.1", "id", "123")) Review Comment: Yes. And It works, but it's not because of the registry mechanism. It works by utilising jackson fallback, where it serialises pojo by using reflections. It doesn't use the alternative EventSerializer in any way and doesn't use it's impelmentation of serializing the Message object. If you expect it to use the provided serialisers then the whole architecture of this feature should be different (and it sounds like reinventing or just exposing the Jackson actually) Currently the limitation comes from the way EventSerializer is defined: it handles the Event as whole, which is an interface that can be extended randomly and have any kinds of data it can imagine. It does not delegates it's pieces some other "XSerializer". Implementing it to delegate any random class to provided serialiser for this specific class sounds like reinventing the Jackson or any other library out there. Alternatively what we could do is dump EventSerializerRegistry, and have a single implementation EventSerializer based on specific implementation: we will have a JacksonEventSerializer which consumes the modules as it designed now, and the extension point will be those modules. The EventSerializerRegistry is not needed for this mechanism of extension but we are also exposing jackson directly as the primary extension mechanism. Which approach do you think best? You can review the changes I did. To review: * the EventSerializerRegistry only picks up serializing Event which is of specific subclass of Event interface. It does not picks up random types of data that is holded by EventImpl. So adding a custom data to fields relies on pojo/annotation mechanism of Jackson or any other implementation of choice inside of the EventImplSerializer. * we can support both usecases of having a custom Event implementation and custom data inside of Event only by exposing JacksonEventSerializer and the Jackson itself because it directly consumes Jackson modules and JacksonSerializers. We can get rid of EventSerializerRegistry then. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
