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

Add an option to skip feature installation #1646

Closed
Kiiv opened this issue Feb 27, 2023 · 13 comments · Fixed by #1672
Closed

Add an option to skip feature installation #1646

Kiiv opened this issue Feb 27, 2023 · 13 comments · Fixed by #1672
Assignees

Comments

@Kiiv
Copy link

Kiiv commented Feb 27, 2023

Hi,

don't know if it's possible, but I'm wondering if it could be possible to add an option to skip features installation goal when running the plugin.
This goal is pretty time consuming and if we launch a full Open Liberty Runtime all features should already be installed.

@scottkurz
Copy link
Member

scottkurz commented Feb 27, 2023

Thanks for raising this issue. Agree this is an important issue, however we wouldn't need an option if we could make the underlying install features routine do a better job no-op'ing when it detects no new features need to be installed. This issue #1557 was recently opened on that point.

@Kiiv
Copy link
Author

Kiiv commented Feb 27, 2023

Thanks for your quick reply ! Yes, it's an other option indeed :D

@scottkurz
Copy link
Member

scottkurz commented Mar 18, 2023

In the meantime, let me leave a note here that the best way currently to skip feature installation within dev mode would probably look something like this:

            <plugin>
                <groupId>io.openliberty.tools</groupId>
                <artifactId>liberty-maven-plugin</artifactId>
                <version>3.7.1</version>
                <executions>
                    <execution>
                        <phase>none</phase>
                        <id>install-feature</id>
                        <goals>
                            <goal>install-feature</goal>
                        </goals>
                        <configuration>
                            <skip>${skip.install.feature}</skip>
                        </configuration>
                    </execution>
                </executions>

So you'd run with mvn liberty:dev -Dskip.install.feature

(This could be extended/modified to skip the goal when run as part of a lifecycle phase if desired).

@scottkurz
Copy link
Member

scottkurz commented Apr 13, 2023

Noticing the performance has improved substantially and consistently for me using the latest runtime version of 23.0.0.3. Real time for mvn liberty:install-feature (for already-installed set of five features) drops from about 15s to 7-8s. Wondering if others see similar results.

@scottkurz
Copy link
Member

We discussed this some internally and think the most useful config option would be something like

"skip install-feature on dev mode start except for immediately after install"

That is, this would be a dev mode only behavior. It would have no impact on the case where dev mode decides to call install-feature because it detects a server.xml, pom.xml, etc. change (in the feature set). It would only apply on dev mode startup. On top of all that, we build in an exception for the very first time install-feature AFTER A NEW LIBERTY INSTALL on dev mode start.... the exception is that in this case, we will call install-feature anyway, even if the config says to skip.

If not for this last exception, the user starting from the (default) kernel runtime artifact would be forced to start with this new skip set to 'false', and then toggle it on to 'true' from thereafter.

The naming of this will be a bit of a challenge (need to condense all the above into a single name).

One consequence of running with this new option would be in the case where you run dev mode, stop it, edit the server.xml adding a feature, and then start dev mode again. In this scenario running with the skip would prevent your new additional feature from getting installed. (This is enough of a surprise IMHO that we didn't really consider making this new skip behavior a new default.)

Finally, there is one corner case where you trigger a new Liberty install later on in your dev mode session (after start). E.g. say you change the runtime version forcing a stop/reinstall/new-start of the server. It seems install-feature should be called in this case. So the exception above would apply to this case as well...we install features even if the new skip is set. However... if this is problematic or hard to detect... (I wouldn't think it would be)... we might be OK not handling this as well... it's unlikely to be used all that often. Anyway, making a note to follow-up and be clear to cover this case when doing the implementation.

@cherylking cherylking self-assigned this May 5, 2023
@cherylking
Copy link
Member

How does skipFeatureInstallOnRestart or skipInstallFeatureOnRestart sound for the new configuration parameter on the dev goal?

@scottkurz
Copy link
Member

How does skipFeatureInstallOnRestart or skipInstallFeatureOnRestart sound for the new configuration parameter on the dev goal?

I like those names... the second seems more logical, (since the goal is 'install-feature') though I like the way the first sounds to my ears, for what that's worth.

CORNER CASE

The 'restart' word in there makes me think we should clarify one corner case....say someone does:
mvn liberty:create liberty:install-feature liberty:dev

I would say that in this case we do skip if the new parm is set. That is, even though it's not a "restart" per se, it is a case where the current 'dev' goal execution did not install this Liberty, so the skip should apply.

What do you think?

(Do I think this corner case is especially likely? No..just an opportunity to clarify the design more precisely. I still think the names are good and this case isn't a strike against those names.)

@cherylking
Copy link
Member

Thanks for the feedback Scott. I'm good with either name.

I agree that the example corner case you gave should skip the install-feature since the Liberty runtime was already installed when the dev goal was called. It seems like we should presume that if the Liberty runtime was not installed by the current invocation of the dev goal, then we honor the skip parameter. We could make sure to document it that way.

@cherylking
Copy link
Member

Noting here that when the create goal is called and the Liberty runtime is already installed, it logs the following message:

CWWKM2107I: Installation type is pre-existing; skipping installation.

and it sets the installType to InstallType.ALREADY_EXISTS.

@cherylking
Copy link
Member

cherylking commented May 10, 2023

Do we need a toggle/hot key for this? I'm thinking just a configuration parameter should be sufficient.

==> Decided no hot key needed.

@cherylking
Copy link
Member

cherylking commented May 10, 2023

@Kiiv Looking for user feedback on the name of the configuration parameter. Some of us like skipInstallFeatureOnRestart and others prefer skipInstallFeature. Here is what the description would look like in the docs.

Screenshot 2023-05-10 at 1 41 51 PM

Those against the longer name think it is confusing since there are two concepts of restart:

  1. When dev mode detects a change and restarts the server automatically.
  2. When the user stops dev mode and starts it again manually.

Another possibility could be skipInstallFeatureOnReRun. Please let us know what you think.

@Kiiv
Copy link
Author

Kiiv commented May 10, 2023

Hi ! Thanks for working on this! 😉
I would prefer skipInstallFeature because, as mentioned, "OnRestart" could be confusing.

I should also propose "skipInstallFeatureOnNextStart" but I don't know if it's more understandable nor too long.

@scottkurz
Copy link
Member

scottkurz commented Jan 31, 2024

Just following up here by observing that with 23.0.0.8 we seem to have a noticeable improvement in performance of mvn liberty:install-feature when the feature set is already installed, presumably because of OpenLiberty/open-liberty#21992. Not sure my data is organized enough to bother including... but I'm seeing 7-9s going down to 2-3s, for what that's worth. I think this option is less interesting now, accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants