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

support lazy, fail-fast and background initialization of asyncapi #297

Merged
merged 10 commits into from Jul 28, 2023

Conversation

tvahrst
Copy link
Contributor

@tvahrst tvahrst commented Jul 21, 2023

This PR adresses issue #292 (provide option to create asyncapi lazy).

It introduces a new Property springwolf.loading-mode with three possible values:

  • FAIL_FAST (AsyncAPI init during spring context creation, same behavior as before)
  • LAZY (AsyncAPI init on first invocation of AsyncApiService.getAsyncApi().
  • BACKGROUND (Same as LAZY, but first invocation is triggered by background thread after 'applicationReay'.

The default value ist LAZY

Some points that might be worth discussing:

  • I was not sure about the term 'loading-mode', another possible name could be 'initializing-mode' ?
  • DefaultChannelService is stateless now, it does not cache the detected ChannelItems. I changed the service's method from getChannels() to findChannels() to avoid 'getter' semantics, which could be interpreted as cheap 'get' operations. findChannels always invokes the scanners to aquire the available channels.
    • additionally, invoking the DefaultChannelService has a sideeffect on SchemasService. SchemasService is a kind of registry, invoked by scanners and caching all detetected schemas. Repeated findChannels() calls replaces the schemas in SchemaService. This is currently no problem, because findChannels() is only called once and SchemasService can handle multiple registrations of the same schema. If that seems to be a risk, DefaultChannelService should assure to call all scanners only once.
    • Exceptions during AsyncAPI detection are cached and thrown (wrapped by a root Exception) on every getAsyncApi call.

@netlify
Copy link

netlify bot commented Jul 21, 2023

Deploy Preview for springwolf-ui canceled.

Name Link
🔨 Latest commit 74a38de
🔍 Latest deploy log https://app.netlify.com/sites/springwolf-ui/deploys/64c39c4909b8ac000812cfb5

Copy link
Member

@timonback timonback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned before, I like the change.

It may look like a lot of comments, but it is mostly nitpicking of details.

  1. I like the 3 options and view failFast and background as essential. lazy comes for free, but I wonder if we need it. I am thinking from the point of view of maintaining it. When it is out there, we cannot change/remove it. Is there a benefit to have lazy in comparison on background?
  2. Personally, I view the feature as an expert/optimization setting. Therefore I would go with failFast as the default and opt in to other modes (and documented it on the website).
    Where am I coming from? As a first time user, I want to quickly see if I have configured everything right. When the spring application starts, that's my indicator that everything is fine (I might not even view logs by default). Delaying it could create user frustration as it is not visible why the application is starting (and the ui showing), but the document is empty and/or has a REST endpoint exception.
  3. Good question regarding the naming. I can imagine also startUpMode and probably prefer init-mode / initMode out of the 3 options.
  4. A stateless ChannelsService is great, as well as the improved naming of the methods. Yes, there is a side-effect, but I do not view it as a risk at this point.
  5. Caching the getAsyncApi result is great, including the behaviour to rethrow the cached exception.
  6. The word detect indicates to me that some is found/discovered (like the channels). For the AsyncApi document, I think it is rather build/constructed/generated.

I might be in the wrong, so feel free to ask questions and/or explain why another way/your suggestions is better.

Thanks again for the contribution.

@tvahrst
Copy link
Contributor Author

tvahrst commented Jul 25, 2023

Good points. I will try to apply the suggested modifications...

tvahrst and others added 2 commits July 27, 2023 17:28
Co-authored-by: Timon Back <timonback@users.noreply.github.com>
…ground' and 'fail_fast'. Moved Initialization completley into AsyncApiInitApplicationListener. Fixed some tests.

Took 47 minutes
@tvahrst
Copy link
Contributor Author

tvahrst commented Jul 27, 2023

I changed 'LoadingMode' into 'InitMode'. Removed 'lazy', so there are two options left: 'background' and 'fail_fast'. Default is 'fail_fast'. Moved the initialization completely into the AsyncApiInitApplicationListener, removed afterPropertiesSet in DefaultAsyncApiService.

Cleaned up some tests.

@timonback
Copy link
Member

The changes look good to me.

I made some changes so that initMode=lazy actually fails the start of the spring boot application: LVM-IT#1
What do you think, shall we merge it? Or do you want to avoid the InitializingBean interface?

feat(core): Ensure initMode=lazy stops application start
@timonback
Copy link
Member

Documentation: springwolf/springwolf.github.io#49

@sam0r040 sam0r040 merged commit 06069cb into springwolf:master Jul 28, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants