-
Notifications
You must be signed in to change notification settings - Fork 182
Feature: Support adding service endpoint after construction (more) #1276
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
Conversation
| se.getMetadata() | ||
| )); | ||
| serialized.set(null); | ||
| return this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- this method should not be public. Only the service code is supposed to build this.
- Clear the lazy serialization. when an endpoint is added
| se.getMetadata() | ||
| )); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to the top of the file
|
|
||
| // build responses first. info needs to be available when adding service endpoints. | ||
| pingResponse = new PingResponse(id, b.name, b.version, b.metadata); | ||
| infoResponse = new InfoResponse(id, b.name, b.version, b.metadata, b.description); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build responses first. info needs to be available when adding service endpoints.
| discoveryContexts = new ArrayList<>(); | ||
| addDiscoveryContexts(SRV_PING, pingResponse, b.pingDispatcher, dTemp); | ||
| addDynamicDiscoveryContexts(SRV_INFO, infoResponse, b.infoDispatcher, dTemp); | ||
| addDiscoveryContexts(SRV_INFO, infoResponse, b.infoDispatcher, dTemp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can do this because of the lazy serialization
| Dispatcher dTemp = null == dInternals || dInternals.isEmpty() ? null : dInternals.get(0); | ||
| EndpointContext ctx = null; | ||
| // do this first so it's available on start | ||
| infoResponse.addServiceEndpoint(se); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do this first so it's available on start
| } | ||
|
|
||
| if (null != infoResponse) { | ||
| infoResponse.addServiceEndpoint(se); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
infoResponse will never be null since it's initialized first in the constructor.
| ServiceMessageHandler handler = smsg -> smsg.respond(conn, sr.serialize()); | ||
| addDiscoveryContexts(discoveryName, dUser, dInternal, handler); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed since lazy serialization
| if (serialized.get() == null) { | ||
| serialized.set(toJson().getBytes(StandardCharsets.UTF_8)); | ||
| } | ||
| return serialized.get(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is part of the JsonSerializable interface, which has a default, always execute implementation. This is why I originally applied the serializer in the add discovery, so it didn't need to be executed every time. But since we can add endpoints, the serialization needs to be updated when endpoints are added.
| Endpoint ep = new Endpoint("endfoo", endMeta); | ||
| ServiceEndpoint se = new ServiceEndpoint(ep, m -> {}, null); | ||
| InfoResponse ir1 = new InfoResponse("id", "name", "0.0.0", metadata, "desc", Collections.singletonList(se)); | ||
| InfoResponse ir1 = new InfoResponse("id", "name", "0.0.0", metadata, "desc").addServiceEndpoint(se); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reflects move the endpoints out of the constructor.
aricart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
Additional testing. Minor tweaks.