Allow plugin reader to ignore dependents of plugins by nineinchnick · Pull Request #12854 · trinodb/trino

nineinchnick · 2022-06-15T08:14:32Z

Description

When the plugin reader is used to map impacted plugins to features, it should not ignore the whole list when the dependents of plugins are included. In Trino that can only be core/trino-server but it currently doesn't define dependencies explicitly, because it would duplicate its Provisio manifest.

Is this change a fix, improvement, new feature, refactoring, or other?
improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)
test tools

How would you describe this change to a non-technical end user or system administrator?
n/a

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

electrum · 2022-06-15T16:52:09Z

Can you give an example scenario that this would improve? Something like "a PR that touches a file in X would previous do Y but now does Z"

nineinchnick · 2022-06-16T16:04:04Z

Can you give an example scenario that this would improve? Something like "a PR that touches a file in X would previous do Y but now does Z"

@electrum let me try:

A PR that adds a dependency on a Trino plugin in any module that's not a plugin would previously disable impact analysis in product tests CI jobs and now the new dependent module can be added to a list of exceptions.

WDYT? We should generally ignore all dependents of plugins but I don't want to pull in Maven code into the Plugin reader and I don't think there's a high chance of any other module than core/trino-server depending on plugins, which I already added in the CI job.

electrum · 2022-06-29T16:12:00Z

I don't think there's a high chance of any other module than core/trino-server depending on plugins

We plan to squash the hive-hadoop2 plugin into the main Hive module. There’s no need at this point for a separate module. At that point, Iceberg and Delta plugins will depend on the Hive plugin.

If the server and/or rpm modules are impacted, the list list of impacted modules should not be empty, because otherwise it will cause all tests to run.

nineinchnick · 2022-10-13T13:08:54Z

@hashhar PTAL, this should fix the recently discovered issue with PRs that only change the trino-server-rpm module.

@findepi PTAL, this should fix running all product tests in PRs.

nineinchnick · 2022-10-13T13:25:22Z

I tested this in nineinchnick#14

findepi · 2022-10-13T14:23:50Z

@findepi PTAL, this should fix running all product tests in PRs.

@nineinchnick thanks!

still, i'd prefer someone else reviews this. I have no prior experience with the plugin reader and surrounding functionalities.

nineinchnick · 2022-10-13T14:29:05Z

@findepi I extracted that commit into #14624

hashhar

making sure i understand things correctly

hashhar · 2022-10-13T18:44:53Z

testing/trino-plugin-reader/src/main/java/io/trino/server/PluginReader.java

        Stream<Map.Entry<String, String>> modulesStream = requireNonNull(modulesToPlugins).entrySet().stream();
        if (impactedModules.isPresent()) {
-            List<String> nonPluginModules = impactedModules.get().stream().filter(module -> !modulesToPlugins.containsKey(module)).collect(Collectors.toList());
+            List<String> nonPluginModules = impactedModules.get().stream()


Probably best to rename this variable since it's no longer just nonPluginModules.

Also add context to commit message of " Allow plugin reader to ignore dependents of plugins "

IIUC this means we now apply impact analysis in product tests if change is in core/trino-server or core/trino-server-rpm whereas earlier we'd have just run all tests.

That brings up concern - what happens when in future some plugin module gets used as dependency in other plugin module?

It might not be as concerning since the only impact would be we'd run more product tests than needed?

hashhar · 2022-10-13T18:48:25Z

.github/workflows/ci.yml

        run: |
          export MAVEN_OPTS="${MAVEN_INSTALL_OPTS}"
-          $MAVEN validate ${MAVEN_FAST_INSTALL} ${MAVEN_GIB} -Dgib.logImpactedTo=gib-impacted.log -pl '!:trino-docs,!:trino-server-rpm'
+          $MAVEN validate ${MAVEN_FAST_INSTALL} ${MAVEN_GIB} -Dgib.logImpactedTo=gib-impacted.log -pl '!:trino-docs'


the list list in commit message for "Don't exclude server modules when checking impacted modules"

I extracted this commit into #14632

hashhar · 2022-10-13T18:48:39Z

.github/workflows/ci.yml

      - name: Build PT matrix (all)
        if: |
-          github.event.name != 'pull_request' ||
+          github.event_name != 'pull_request' ||


this was extracted - remove this commit?

nineinchnick · 2022-10-14T08:44:19Z

I closed this because after giving it another thought I don't think we should be doing impact analysis when we know that some dependants of plugins have changed.

cla-bot bot added the cla-signed label Jun 15, 2022

nineinchnick added the no-release-notes This pull request does not require release notes entry label Jul 18, 2022

nineinchnick force-pushed the plugin-reader-ignore-dependents branch from 4fabc4c to cb9f80b Compare September 22, 2022 07:50

nineinchnick force-pushed the plugin-reader-ignore-dependents branch from cb9f80b to 095d285 Compare October 13, 2022 10:40

This was referenced Oct 13, 2022

[TEST] Plugin reader ignore dependents test #14617

Closed

[TEST] Don't run all tests with changes only in RPM nineinchnick/trino#14

Closed

Use JDK 17 based image for Server IT #14614

Merged

nineinchnick force-pushed the plugin-reader-ignore-dependents branch from 095d285 to 7590818 Compare October 13, 2022 11:42

Jan Was added 2 commits October 13, 2022 13:54

Allow plugin reader to ignore dependents of plugins

1521143

Don't exclude server modules when checking impacted modules

60fdd2f

If the server and/or rpm modules are impacted, the list list of impacted modules should not be empty, because otherwise it will cause all tests to run.

nineinchnick force-pushed the plugin-reader-ignore-dependents branch from 7590818 to 60fdd2f Compare October 13, 2022 11:54

Use correct variable for event name in CI

e97dc51

nineinchnick mentioned this pull request Oct 13, 2022

Improve performance of Iceberg remove_orphan_files #14383

Merged

nineinchnick requested review from ebyhr, findepi and hashhar October 13, 2022 13:09

hashhar reviewed Oct 13, 2022

View reviewed changes

nineinchnick closed this Oct 14, 2022

nineinchnick deleted the plugin-reader-ignore-dependents branch May 27, 2024 08:19

Conversation

nineinchnick commented Jun 15, 2022

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

electrum commented Jun 15, 2022

Uh oh!

nineinchnick commented Jun 16, 2022

Uh oh!

electrum commented Jun 29, 2022

Uh oh!

nineinchnick commented Oct 13, 2022

Uh oh!

nineinchnick commented Oct 13, 2022

Uh oh!

findepi commented Oct 13, 2022

Uh oh!

nineinchnick commented Oct 13, 2022

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

hashhar Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

hashhar Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

hashhar Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

hashhar Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

nineinchnick Oct 14, 2022

Choose a reason for hiding this comment

Uh oh!

hashhar Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

nineinchnick commented Oct 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants