Open-EO / openeo-js-client

JavaScript and TypeScript client for the openEO API.
https://open-eo.github.io/openeo-js-client/latest/
Apache License 2.0
15 stars 6 forks source link

Current state in monitorJob #26

Closed jneijt closed 3 years ago

jneijt commented 3 years ago

Currently one has to wait the set interval until the first logs are available. Especially when the interval is set to a higher timeout, the user has no feedback at the beginning. This means that if we were to show the logs in a UI somewhere, we have to kind of copy the functionality of the monitorFn in monitorJob and run it ourselves before starting monitorJob to be able to show something until the first timeout. This would work of course but I personally think it would be a nice addition if the monitorJob function would take care of this already.

What do you think?

m-mohr commented 3 years ago

I had that implemented first (by having let lastStatus = null;), but then thought it is likely not necessary as I thought it's not very useful to get a callback if there's no logs available. If there's logs available then it runs anyway at the beginning. If you need a first call to the callback at the beginning always, why not just call your callback once yourself? That way you have full control over the flow. So I'm not 100% sure what the benefit is here? Is it to get the Job infos from describeJob? Then that would make sense.

m-mohr commented 3 years ago

Ah, wait. Now I understand... the interval doesn't execute the callback at the start. It always waits the interval once... Okay, makes much sense! Should it always run, also if there are no logs?

m-mohr commented 3 years ago

Actually I just found that I mistakenly used setTimeout instead of setInterval. See #28

jneijt commented 3 years ago

Exactly, the interval doesn't execute the callback in the beginning, only after the timeout has passed once. The way I was using it now was:

  1. Fetch the job infos using describeJob to prepare a simple overview over the job in general
  2. Start monitorJob and in my callback add the logs and (if available) results to that page

In this case, the page would not show any logs until the interval has passed once. Of course I could go and fetch the logs myself once but that somehow felt a bit like a duplicate.

I don't think it needs to run if there are no logs though, it doesn't really fetch any new information then, right? But do you know if there are any logs before running the monitorFn at least once (which already does what we want)?

m-mohr commented 3 years ago

Here would be my approach: https://github.com/Open-EO/openeo-js-client/pull/28 Would that work for you and help you out?

The question is whether it needs to run once just to call describeJob... I guess not? At least createJob also calls it.

But do you know if there are any logs before running the monitorFn at least once (which already does what we want)?

Could be.

jneijt commented 3 years ago

Aaah, now I get the point with the lastStatus. I completely missed that, makes sense now. I'll quickly test your proposed solution, looks like it should work.

jneijt commented 3 years ago

Yep, looks good from my side and the quick test did exactly what I was looking for.

Is this now also according to what you intended with lastStatus?

I don't think it has to run the callback just for the sake of calling describeJob, I think you probably already have that info before calling monitorJob and otherwise it's easy to add just one call to that manually in your own code. But for the logs it's nice to have.

m-mohr commented 3 years ago

Hmm... I'm back and forth with it. For some use cases it's useful to call describeJob, for some not so much. So I guess I just leave it as it is in the PR, which means the callback is always called once at the beginning with the updated job details.

jneijt commented 3 years ago

I don't think it hurts to run the callback with an updated job and no logs in the beginning, even if this is the exact same job which the user (maybe) already has. So the PR should be fine I think. On the other hand, just calling it whenever the status has changed or if there are new logs seems reasonable to me as well. However, I'm not sure I understand all the use cases here yet.

Which use case would benefit from getting the updated job in any case right away? When you create the Job object directly with new Job(connection, id) instead of retrieving it with something like connection.getJob(id)?

m-mohr commented 3 years ago

The use case I'm thinking about is the Web Editor, where the user starts monitorJob maybe just a minute after creating the job. For other use cases where you just start monitoring directly after creating the job, it's likely not getting you a different result.

m-mohr commented 3 years ago

Okay, I just implemented the approach that describeJob is only called at the beginning if the data has not been refreshed recently (in the last second) and then it calls the callback if the status changed or logs are available (i.e. older than the time specified in the interval). I think that caters for both use cases best.

m-mohr commented 3 years ago

Has been merged. I'll make a rc3 release later today.

m-mohr commented 3 years ago

The new version has been pushed to npm.

jneijt commented 3 years ago

Just updated the app to the newest version locally and this works perfectly, thanks a lot! 👍