Remove most the Guava usage #159

olamy · 2021-06-17T01:01:47Z

start work on get rid of guava...
remove most of the guava usage

dwnusbaum · 2021-06-17T13:11:49Z

src/main/java/org/jenkinsci/plugins/workflow/actions/ArgumentsAction.java

            return false;
        }
-        if (o == null || Primitives.isWrapperType(o.getClass()) || o.getClass().isEnum()) {
+        if (o == null || o.getClass().isPrimitive() || o.getClass().isEnum()) {


I think these have different meanings. The old Guava code is true for boxed types like Integer or Double, whereas the new code is true for primitive types like int or double.

dwnusbaum · 2021-06-17T13:15:02Z

src/main/java/org/jenkinsci/plugins/workflow/actions/ArgumentsAction.java

            return Collections.emptyMap();
        }
-        HashMap<String, Object> filteredArguments = Maps.newHashMapWithExpectedSize(internalArgs.size());
+        HashMap<String, Object> filteredArguments = new HashMap(internalArgs.size());


Note that there is a subtle difference between these, see the discussion of loadFactor in the HashMap documentation.

Yes I know but looking at the context. It probably doesn't hurt...

HashMap<String, Object> filteredArguments = new HashMap(internalArgs.size()); for (Map.Entry<String, Object> entry : internalArgs.entrySet()) { if (entry.getValue() != null && !(entry.getValue() instanceof NotStoredReason)) { // TODO this is incorrect: value could be a Map/List with some nested entries that are NotStoredReason filteredArguments.put(entry.getKey(), entry.getValue()); } }

If 25% of the internalArgs Map is filtered by the if condition this optimisation with size/loadfactor doesn't make any sense.
and by the way looking at the rest of the code this Map is only read.
I might more using something such?:

Map<String, Object> filteredArguments = internalArgs.entrySet().stream() .filter(entry -> entry.getValue() != null && !(entry.getValue() instanceof NotStoredReason)) .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));

dwnusbaum · 2021-06-17T13:16:41Z

src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/AbstractFlowScanner.java

    public FlowNode findFirstMatch(@CheckForNull Collection<FlowNode> heads,
-                                           @CheckForNull Collection<FlowNode> blackListNodes,
-                                           Predicate<FlowNode> matchCondition) {
+                                   @CheckForNull Collection<FlowNode> blackListNodes,
+                                   Predicate<FlowNode> matchCondition) {


FWIW, I think changing signatures like this from Guava's Predicate to Java 8's Predicate will be a breaking change for some plugins, so you might need to introduce the new signature while keeping the old one around for a few releases to give you time to update other plugins.

Yup agree, it's a bit too agressive change... Even if I checked with usage-in-plugins and there is not so much usage of that.
Interesting if the code is using only lambda maybe is not a problem (at least at compile time). But yup will not work at runtime :)
but yeah might be better to keep both (but that's a lot... especially a lot are already marked deprecated..)

Yeah, I think a lot of cases would be fine, but stuff like this along with callers like this would need to be updated.

Signed-off-by: olivier lamy <olamy@apache.org>

Signed-off-by: Olivier Lamy <olamy@apache.org>

dwnusbaum

I think there is one change here that will break workflow-cps's tests in the PCT, and there are a few breaking changes that I think are probably safe in practice but that I would recommend either making them non-breaking or taking them a bit further to clean up a few more uses of Guava.

dwnusbaum · 2021-06-28T13:24:36Z

src/main/java/org/jenkinsci/plugins/workflow/actions/ArgumentsAction.java

-        HashMap<String, Object> filteredArguments = Maps.newHashMapWithExpectedSize(internalArgs.size());
-        for (Map.Entry<String, Object> entry : internalArgs.entrySet()) {
-            if (entry.getValue() != null && !(entry.getValue() instanceof NotStoredReason)) {
-                // TODO this is incorrect: value could be a Map/List with some nested entries that are NotStoredReason


nit: I think this comment is still accurate, right? If so I would add it back to the new code.

dwnusbaum · 2021-06-28T13:26:16Z

src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/AbstractFlowScanner.java

        return this;
    }

+


Suggested change

dwnusbaum · 2021-06-28T13:33:14Z

src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/Filterator.java

+
    /** Returns a filtered view of the iterator, which calls the iterator until matches are found */
    @Nonnull
    Filterator<T> filter(@Nonnull Predicate<T> matchCondition);


Adding a new non-default method to an interface is a breaking change for any classes that implement this interface, but from some quick searching I don't see any such implementations outside of this plugin.

I would either change this to a default method that delegates to the existing implementation to avoid breaking changes, or if you are fine with the breaking change as long as there is no impact, I would double-check for existing implementors with a tool like https://github.com/jenkins-infra/usage-in-plugins and then drop Guava entirely.

dwnusbaum · 2021-06-28T13:36:29Z

src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/FlowScanningUtils.java

-                return (input != null && input.getAction(actionClass) != null);
-            }
-        };
+    public static Predicate<FlowNode> hasActionAsPredicate(@Nonnull final Class<? extends Action> actionClass) {


Hmm, it's awkward to have to add a new method but we have no choice here. There will need to be quite a few downstream updates for this one: https://github.com/search?utf8=%E2%9C%93&q=user%3Ajenkinsci+hasActionPredicate&type=Code.

dwnusbaum · 2021-06-28T13:46:07Z

src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/NodeStepNamePredicate.java

 import javax.annotation.Nullable;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
+import java.util.function.Predicate;


Changing the class like this is a breaking change, it will break test code like this in the PCT: https://github.com/jenkinsci/workflow-cps-plugin/blob/ac55a171db521d1cad46b7136715db7beefcda61/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java#L237-L239.

dwnusbaum · 2021-06-28T13:49:36Z

src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/AbstractFlowScanner.java

+    @Deprecated
+    public FlowNode findFirstMatch(@CheckForNull Collection<FlowNode> heads,


nit: For all of the methods you are deprecating, can you please add a Javadoc comment like @deprecated use {@link #newMethod(foo, Predicate)}?

dwnusbaum · 2021-06-28T13:56:25Z

src/main/java/org/jenkinsci/plugins/workflow/graphanalysis/ForkScanner.java

 import java.util.List;
 import java.util.ListIterator;
 import java.util.Set;
+import java.util.function.Predicate;


For whole-class changes like this, did you double-check to make sure that all of the methods in this class and other subclasses of AbstractFlowScanner that either return Predicate or use it for a parameter are overriding a method from AbstractScanner where you added a new overload using Guava's predicate to retain backwards compatibility?

For example, I think ForkScanner#setParallelStartPredicate is actually specific to ForkScanner, so this is a breaking change for anyone using that method unless you update it to use Guava's Predicate (but I don't see any callers and that method was already deprecated, so maybe it should just be deleted?).

timja · 2022-01-10T13:01:03Z

@olamy do you plan to revisit this? (hit in jenkinsci/junit-realtime-test-reporter-plugin#33 (comment))

jglick · 2022-07-01T14:47:12Z

Conflicts with #177.

olamy · 2022-07-05T11:18:11Z

old work. better to forget this.

olamy added the dependencies label Jun 17, 2021

dwnusbaum reviewed Jun 17, 2021

View reviewed changes

olamy added 5 commits June 26, 2021 16:20

start work on get rid of guava...

78bd678

Signed-off-by: olivier lamy <olamy@apache.org>

remove most of the guava usage

09b3262

Signed-off-by: olivier lamy <olamy@apache.org>

fix from review

b96ecb6

Signed-off-by: olivier lamy <olamy@apache.org>

restore some api with guava but deprecated

19c7d82

Signed-off-by: Olivier Lamy <olamy@apache.org>

fix unit test

e153c46

Signed-off-by: Olivier Lamy <olamy@apache.org>

olamy force-pushed the get-rid-of-guava branch from 8bd6539 to e153c46 Compare June 26, 2021 06:20

this one is not deprecated

2406633

Signed-off-by: Olivier Lamy <olamy@apache.org>

olamy marked this pull request as ready for review June 26, 2021 06:24

keep backward compat

3d10d6c

Signed-off-by: Olivier Lamy <olamy@apache.org>

batmat requested review from bitwiseman, car-roll and dwnusbaum June 28, 2021 08:41

dwnusbaum reviewed Jun 28, 2021

View reviewed changes

rsandell requested review from jtnord and rsandell June 29, 2021 08:40

jglick mentioned this pull request Nov 1, 2021

Reduce usages of Guava #177

Merged

olamy marked this pull request as draft November 18, 2021 00:59

timja mentioned this pull request Jan 10, 2022

adding API to return test progress based on previous run results jenkinsci/junit-realtime-test-reporter-plugin#33

Closed

6 tasks

olamy closed this Jul 5, 2022

olamy deleted the get-rid-of-guava branch July 5, 2022 11:18

		@Deprecated
		public FlowNode findFirstMatch(@CheckForNull Collection<FlowNode> heads,

Remove most the Guava usage #159

Remove most the Guava usage #159

Uh oh!

Conversation

olamy commented Jun 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwnusbaum Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olamy Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwnusbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timja commented Jan 10, 2022

Uh oh!

jglick commented Jul 1, 2022

Uh oh!

olamy commented Jul 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dwnusbaum Jun 17, 2021 •

edited

Loading

olamy Jun 18, 2021 •

edited

Loading