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

Improve Deserialization of InstanceEvents #1348

Closed
ChunMengLu opened this issue Jan 19, 2020 · 7 comments
Closed

Improve Deserialization of InstanceEvents #1348

ChunMengLu opened this issue Jan 19, 2020 · 7 comments

Comments

@ChunMengLu
Copy link

Bug report
I customized eventstore and found that it is not good enough for deserialization.
I use jackson and need to customize a Deserializer like RegistrationDeserializer.

@joshiste
Copy link
Collaborator

Could you please elaborate a bit on your customizing and why you need to deserialize the InstanceEvents using jackson?
What changes to the InstanceEvents would you apply to improve it?

@ChunMengLu
Copy link
Author

Because the memory currently used for InstanceEvents storage, spring boot admin will be lost after restart, so I use rocksdb to store. Use jackson for serialization.

@ChunMengLu
Copy link
Author

Currently I use custom deserialization:

/**
 * InstanceEvent Deserializer
 *
 * @author L.cm
 */
public class InstanceEventDeserializer extends StdDeserializer<InstanceEvent> {

	public InstanceEventDeserializer() {
		super(InstanceEvent.class);
	}

	@Override
	public InstanceEvent deserialize(JsonParser jsonParser, DeserializationContext context) throws IOException {
		JsonNode node = jsonParser.readValueAsTree();
		String type = node.at("/type").asText();
		InstanceId instance = InstanceId.of(node.at("/instance").asText());
		long version = node.at("/version").asLong();
		Instant timestamp = Instant.parse(node.at("/timestamp").asText());

		if ("DEREGISTERED".equals(type)) {
			return new InstanceDeregisteredEvent(instance, version, timestamp);
		} else if ("ENDPOINTS_DETECTED".equals(type)) {
			List<Endpoint> endpoints = JsonUtil.convertValue(node.at("/endpoints"), JsonUtil.getListType(Endpoint.class));
			return new InstanceEndpointsDetectedEvent(instance, version, timestamp, Endpoints.of(endpoints));
		} else if ("INFO_CHANGED".equals(type)) {
			Map<String, Object> value = JsonUtil.convertValue(node.at("/info"), Map.class);
			return new InstanceInfoChangedEvent(instance, version, timestamp, Info.from(value));
		} else if ("REGISTERED".equals(type)) {
			Registration registration = JsonUtil.convertValue(node.at("/registration"), Registration.class);
			return new InstanceRegisteredEvent(instance, version, timestamp, registration);
		} else if ("REGISTRATION_UPDATED".equals(type)) {
			Registration registration = JsonUtil.convertValue(node.at("/registration"), Registration.class);
			return new InstanceRegistrationUpdatedEvent(instance, version, timestamp, registration);
		} else if ("STATUS_CHANGED".equals(type)) {
			Map<String, Object> value = JsonUtil.convertValue(node.at("/statusInfo"), Map.class);
			return new InstanceStatusChangedEvent(instance, version, timestamp, StatusInfo.from(value));
		}
		return null;
	}
}

@joshiste joshiste changed the title InstanceEvent is not friendly enough for deserialization。 Improve Deserialization of InstanceEvents Jan 20, 2020
@joshiste
Copy link
Collaborator

@ChunMengLu Currently I think the best is to provide a jackson module with your deserializer by SBA as I'm currently refusing to taint the code with the jackson polymorphic deserialization stuff.
Do you want to work on a PR for this?

@srempfer
Copy link
Contributor

I started the development of #684 and as pre-requirement I need a Jackson module for the InstanceEvent stuff.
I'll contribute the module as separate PR for this issue because I not know how long the Redis based event store will take to implement.

srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 24, 2020
@srempfer
Copy link
Contributor

srempfer commented Apr 24, 2020

I just referenced a working state to the issue.

@joshiste: Is that what you had in mind? Do you prefer...

  • ...the Mixin variant like I did it for InstanceEventMixin/InstanceEndpointsDetectedEventMixin
  • ...the @jsonxyz annotations direct in the domain classes like I did for InstanceRegistrationUpdatedEvent
  • ... a deserializer like the above InstanceEventDeserializer

My preference is the own "InstanceEventDeserializer" because of the minimal changes required.
It also don't pollutes the domain classes with the jackson stuff and you don't need a mixin for every InstanceEvent.
In case of get it not proper working with the 'InstanceEventDeserializer' approach I would create a mixin for the polymorphic stuff and add 'JsonCreator' annotations to the events like its already done for InstanceId

srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 25, 2020
srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 25, 2020
joshiste pushed a commit that referenced this issue Apr 27, 2020
* Fix code style issue

* Create own Jackson module for Spring Boot Admin Server

preparation for #1348
@joshiste
Copy link
Collaborator

joshiste commented Apr 27, 2020

My preference would be to pollute the core domain as little as possible with Jackson annotations. So either a custom deserializer or a mixin would be fine.

srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 27, 2020
srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 28, 2020
srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 28, 2020
srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 28, 2020
srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 28, 2020
srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 28, 2020
srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 28, 2020
srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 28, 2020
srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 29, 2020
srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 29, 2020
srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 29, 2020
srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 29, 2020
srempfer added a commit to srempfer/spring-boot-admin that referenced this issue Apr 30, 2020
@joshiste joshiste added this to the 2.2.3 milestone May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants