Implement SSE #90
Implement SSE #90
Conversation
cdr-chakotay
added
enhancement
sdks
cdr-chakotay
force-pushed
the
Implement_SSE
branch
from
b987563
to
217ec4d
Compare
|
Let me know if you need some more assistance here :) |
Hey @Acconut thank you for the reminder! I was occupied with other work in the meantime. I would like to continue on this soon. I let you know if I need anything :) |
cdr-chakotay
force-pushed
the
Implement_SSE
branch
from
61bb0dc
to
481edd5
Compare
| * @param combinedProgress Overall execution progress of the whole assembly. | ||
| * @param progressPerOriginalFile JSONObject including the progress for each individual file. | ||
| */ | ||
| void onAssemblyProgress(double combinedProgress, JSONObject progressPerOriginalFile); |
There was a problem hiding this comment.
Does every user of the Java SDK now have to update their listeners when they upgrade the Java SDK? If so, this would be a breaking change.
There was a problem hiding this comment.
Will be a braking change, yes. Intended to push out all of this in a major.
I thought it might make more sense to include new possible features, which SSE provides.
| assemblyListener.onAssemblyUploadFinished(); | ||
| break; | ||
| default: | ||
| LocalOperationException e = new LocalOperationException("Unknown SSE message: " + data); |
There was a problem hiding this comment.
We shouldn't error out on unknown events. In the future, we might add new events and SDKs should not start failing then.
There was a problem hiding this comment.
What do you think about piping this as "onError" Event inside the assembly listener ?
There was a problem hiding this comment.
Unknown events are not an error and shouldn't be forwarded to the user. If SDKs error out on unknown events, we cannot add new ones in the future.
| if (streamEvent != null) { | ||
| if (streamEvent instanceof MessageEvent) { | ||
| handleMessageEvent((MessageEvent) streamEvent); | ||
| } else if (streamEvent instanceof CommentEvent) { |
There was a problem hiding this comment.
Since we don't use CommentEvent, StartedEvent and FaultEvents, can we just remove them here and reduce the LOC? I also don't think we will be used them soon.
There was a problem hiding this comment.
I'd like to keep them, also the started event is sent by the library itself :)
| @@ -0,0 +1,26 @@ | |||
| id: 2b7beaddf0a046e2ada603346f2cbee1 | |||
There was a problem hiding this comment.
Can we give this file a .txt extension?
| Assertions.assertEquals(response.json().get("assembly_id"), "02ce6150ea2811e6a35a8d1e061a5b71"); | ||
| Assertions.assertEquals(response.json().get("ok"), "ASSEMBLY_COMPLETED"); | ||
| } | ||
|
|
||
| @Override | ||
| public void onError(Exception error) { | ||
| emittedEvents.put("ASSEMBLY_ERROR", true); | ||
| System.err.println("No Mockserver Response"); |
There was a problem hiding this comment.
Is this log line still relevant? Can we remove it?
| } | ||
|
|
||
| @Override | ||
| public void onAssemblyResultFinished(String stepName, JSONObject result) { | ||
|
|
||
|
|
||
| emittedEvents.put("ASSEMBLY_RESULT_FINISHED", true); |
There was a problem hiding this comment.
Can you add assertions to the event argments here and in the other methods? Then we can be sure that the data from SSE is passed properly to the listener.
Implement Server Sent Events (SSE) as replacement for socket polling.
This is due to SSE preference from the Transloadit API.