Publish completed step values on close (StepRegistry)

[lenin-jaganathan]

fixes #3863

[shakuzen]

Looks good. I left a nit and a question. This should fix things for the case of closing before scheduled publishing in a step. I think there will still be issues if close happens while scheduled publishing is happening, but we can worry about that separately from this change. That should be a much rarer case.

[shakuzen]

I don't think it's a problem with the current implementation, but it feels like it would be more correct to set this directly from the scheduled task rather than in this method. After all, this method could be called from outside scheduling. I'm thinking updating the scheduling to something like:

scheduledExecutorService.scheduleAtFixedRate(() -> {
    this.lastScheduledPublishStartTime = clock.wallTime();
    publishSafely();
}, initialDelayMillis, stepMillis, TimeUnit.MILLISECONDS);

What do you think?

[lenin-jaganathan]

It has its own drawbacks, we do this check this.publishing.compareAndSet(false, true) in publishSafely which opens up the possibility that either publish can happen or cannot happen when you call publishSafely. So, if it doesn't do a publish then updating lastScheduledPublishStartTime in scheduling might be problematic, because the publish didn't start at that rather it was attempted and turned down.

variable lastScheduledPublishStartTime might be interpreted in 2 ways by its name -

• the time when the publish was called by the scheduling thread. (which is what I wanted to convey)
• but it can also be interpreted as you did above.

The Javadoc for getLastScheduledPublishStartTime() says "Returns the time when the last scheduled publish was started by {@link PushMeterRegistry#publishSafely()}." which I guess represents better. If you feel confused or think otherwise, we can reconsider. But be aware that adding it in scheduleAtFixedRate means it no longer represents the time when publish was called rather it represents when publishSafely was called.

[shakuzen]

Yeah, considering everything identified so far it seems like it will be better to leave it as is for now.

[lenin-jaganathan]

the build failure seems unrelated to this PR and maybe a flaky one or something. I don't have permission to restart the job.