Skip to content

Conversation

@MikeEdgar
Copy link
Contributor

@MikeEdgar MikeEdgar commented Sep 25, 2025

Type of change

  • Refactoring

Description

Project modifications to resolve several errors in Eclipse IDE. This is intended to drive discussion better than an issue without the proposed changes being available.

  • Copy kafka-versions.yaml and bridge.version files during process-resources phase of cluster-operator and systemtests modules. This avoids having the modules reach out into the parent directory for resources.
  • Provide generic argument type hints in several locations in the topic operator's TypeHandler.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md
  • Supply screenshots for visual changes, such as Grafana dashboards

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

This looks pretty messy to be honest.

@MikeEdgar
Copy link
Contributor Author

This looks pretty messy to be honest.

Beauty is in the eye of the beholder 🤣 Anything in particular or all of it?

@scholzj
Copy link
Member

scholzj commented Sep 25, 2025

I think it should keep the removed version files where they were for example.

@scholzj
Copy link
Member

scholzj commented Sep 25, 2025

The assembly stuff is IMHO required for the build process. So not wure what sense does it make to disable it.

@scholzj
Copy link
Member

scholzj commented Sep 25, 2025

Splitting this into individual things and doing them one step at a time might also improve the reviewibility quite significantly.

@MikeEdgar MikeEdgar force-pushed the eclipse-modifications branch from be05de5 to b6459ae Compare September 25, 2025 19:40
@MikeEdgar
Copy link
Contributor Author

The assembly stuff is IMHO required for the build process. So not wure what sense does it make to disable it.

The idea is that it's not really useful when building the project in an IDE. The goal with it is to create archives for distribution, right? Moving to a profile and disabling it means when opening the project you don't need to have those tasks running (assuming the IDE runs them) and prebuilding/installing the project so the dependencies can be copied.

Splitting this into individual things and doing them one step at a time might also improve the reviewibility quite significantly.

I'll do that.

@scholzj
Copy link
Member

scholzj commented Sep 25, 2025

The idea is that it's not really useful when building the project in an IDE. The goal with it is to create archives for distribution, right? Moving to a profile and disabling it means when opening the project you don't need to have those tasks running (assuming the IDE runs them) and prebuilding/installing the project so the dependencies can be copied.

Why would an IDE run a package process at startup? That seems wrong.

@codecov
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.81%. Comparing base (3415bcd) to head (cebe1bc).

Files with missing lines Patch % Lines
...n/java/io/strimzi/operator/topic/KafkaHandler.java 91.66% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #11915      +/-   ##
============================================
+ Coverage     74.79%   74.81%   +0.01%     
+ Complexity     6617     6616       -1     
============================================
  Files           376      376              
  Lines         25325    25325              
  Branches       3389     3389              
============================================
+ Hits          18943    18946       +3     
+ Misses         4996     4994       -2     
+ Partials       1386     1385       -1     
Files with missing lines Coverage Δ
...n/java/io/strimzi/operator/topic/KafkaHandler.java 94.71% <91.66%> (ø)

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@MikeEdgar MikeEdgar force-pushed the eclipse-modifications branch from b6459ae to fedc1c9 Compare September 25, 2025 20:42
Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

@MikeEdgar thanks for this. I think the project should be friendly with other IDEs than IntelliJ. I left a few questions tough trying to understand the meaning of the changes to make it working within Eclipse.

return TopicOperatorUtil.partitionedByError(reconcilableTopics.stream().map(reconcilableTopic -> {
if (newTopicsErrors.containsKey(reconcilableTopic)) {
return new Pair<>(reconcilableTopic, Either.ofLeft(newTopicsErrors.get(reconcilableTopic)));
return new Pair<>(reconcilableTopic, Either.<TopicOperatorException, TopicState>ofLeft(newTopicsErrors.get(reconcilableTopic)));
Copy link
Member

Choose a reason for hiding this comment

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

why these changes? Does Eclipse fail the compile without them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it seems as though Eclipse JDT is not smart enough to determine the generic type arguments on these. I suspect the same errors would appear in VS Code, which at some point also used JDT.

Copy link
Member

Choose a reason for hiding this comment

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

The problem I have with this is that we can fix them now. Then we'll develop more but not using this syntax and then at some point someone using Eclipse will raise the issue. Is there really nothing that can't be done in Eclipse JDT to avoid this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for sure an issue, but as I understand it the JDT compiler is a different implementation aiming for incremental compilation in the editor so it's always possible for these kind of differences. The generic use in this class is probably on the more complex end of the spectrum, so it may not be that common and it could just be dealt with if they occur again.

There are other occurrences of the type params being set already in this class, so it's not unprecedented. Apparently even javac was stunned by it at some point 😆

LOGGER.traceOp("Admin.listPartitionReassignments({}) failed with {}", apparentDifferentRfPartitions, e);
return apparentlyDifferentRfTopics.stream().map(pair ->
new Pair<>(pair.getKey(), Either.<TopicOperatorException, TopicState>ofLeft(handleAdminException(e)))).toList();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, I did find that Intellij can use the Eclipse compiler via Settings > Build, Execution, Deployment > Compiler > Java Compiler.

<includes>
<include>kafka-versions.yaml</include>
</includes>
</resource>
Copy link
Member

Choose a reason for hiding this comment

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

I am struggling to understand why it was removed from here but then having an execution task to copy it, IIUC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, it's due to this bug in Eclipse: eclipse-m2e/m2e-core#1790

Granted, it's Eclipse's bug, but nonetheless an impediment to using that IDE with Strimzi.

@MikeEdgar
Copy link
Contributor Author

Job failure seem unrelated:

tar: c7a4a6b4c0744e558980828618275b4d_archive.tar: Cannot write: No space left on device
tar: Error is not recoverable: exiting now
ApplicationInsightsTelemetrySender correlated 1 events with X-TFS-Session c8e8b170-2700-4668-9cb4-4e70bf2050d4
##[error]Process returned non-zero exit code: 2
Finishing: Maven cache

@MikeEdgar MikeEdgar force-pushed the eclipse-modifications branch from fedc1c9 to 8533192 Compare October 1, 2025 21:19
return TopicOperatorUtil.partitionedByError(reconcilableTopics.stream().map(reconcilableTopic -> {
if (newTopicsErrors.containsKey(reconcilableTopic)) {
return new Pair<>(reconcilableTopic, Either.ofLeft(newTopicsErrors.get(reconcilableTopic)));
return new Pair<>(reconcilableTopic, Either.<TopicOperatorException, TopicState>ofLeft(newTopicsErrors.get(reconcilableTopic)));
Copy link
Member

Choose a reason for hiding this comment

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

The problem I have with this is that we can fix them now. Then we'll develop more but not using this syntax and then at some point someone using Eclipse will raise the issue. Is there really nothing that can't be done in Eclipse JDT to avoid this?

@ppatierno ppatierno requested a review from a team October 2, 2025 17:26
@ppatierno
Copy link
Member

ppatierno commented Oct 2, 2025

The goal of this PR should be to make Strimzi more Eclipse-friendly (we should try the same for VS Code as well). I was wondering what are other @strimzi/maintainers opinions on the needed changes here. There could be users who would like to contribute but they are not allowed to use IntelliJ (while mostly all of us can use it).

@scholzj
Copy link
Member

scholzj commented Oct 2, 2025

The goal of this PR should be to make Strimzi more Eclipse-friendly (we should try the same for VS Code as well). I was wondering what are other @strimzi/maintainers opinions on the needed changes here. There could be users who would like to contribute but they are not allowed to use IntelliJ (while mostly all of us can use it).

I do not have a problem with the goal per se. I think there should be more discussion about what the problems are and what the right solutions are. That will also help us understand the issues Eclipse as, and try to avoid them in the future. But unless someone from the core team uses Eclipses daily, it might be hard to maintain. I also in particular do not like the idea of moving or copying key files around. That does not help transparency or understanding of how things work and make things more complicated for everyone else. I also do not understand the motivation given there never was any previous demand. (Quite frankly, I know some people using VSCode ... but I thought Eclipse users were already extint ;-)) That is important aspect to decide on the trade-offs.

@MikeEdgar
Copy link
Contributor Author

How do we proceed here? As far as discussion of the issues, the ones addressed with this PR are:

  1. Use the maven-resources-plugin to place files from the project root to the build output directories of the cluster-operator and systemtest modules. This works around Eclipse bug java.lang.IllegalArgumentException: Path must include project and resource name: / eclipse-m2e/m2e-core#1790 which does not tolerate a module reaching out to the parent directory to add the root as a resource path.
  2. Add type hints to several uses of Either. My guess is that Eclipse's incremental compiler is unable to infer both of Eithers types given a single argument in ofLeft or ofRight.

It's clear that without core maintainers using a range of development environments, things like this will creep into the project and those affected will need to either work around them locally or propose changes to resolve them. I suppose that's the same as with any piece of code where certain users are affected by edge cases and need to deal with it in some way.

@MikeEdgar MikeEdgar force-pushed the eclipse-modifications branch from e5c09ca to 6573c21 Compare October 16, 2025 10:39
@ppatierno
Copy link
Member

I can understand Jakub's concerns but at the same time I think we should discuss more about this (maybe on a community call) because I think it's important having Strimzi working easily with other IDEs like Eclipse and VSCode.

It's clear that without core maintainers using a range of development environments, things like this will creep into the project and those affected will need to either work around them locally or propose changes to resolve them.

I don't think we should not be strict on what maintainers are using. People can move across companies and can get restrictions on IDEs to use, so what I can use and work today could not be the same tomorrow.

Also working with other IDEs allows to increase the contributors space with people not using IntelliJ. For example, I had a dev within IBM using Eclipse and he has been having the same issue. I pointed him to this PR to understand if the fixes proposed by Michael works for him or there is anything to improve. Others I engaged are using VSCode instead.

What the other @strimzi/maintainers think about the above discussion?

@MikeEdgar
Copy link
Contributor Author

I don't think we should not be strict on what maintainers are using. People can move across companies and can get restrictions on IDEs to use, so what I can use and work today could not be the same tomorrow.

Yes, I think that makes sense. I'd also add that I don't think an individual needs to use a particular IDE daily just to check in on bigger changes, etc. to see if one IDE has many new errors or warnings of some kind. Speaking for myself, although I am not doing much development in Strimzi, I do always have it open in Eclipse and track main for research/interest so if there is a desire to keep things error-free for multiple development environments, I'm certainly willing to raise issues from and Eclipse perspective.

- use maven-resources-plugin/copy-resources instead of referencing
resources outside of module project directories
- provide generic argument type hints in several locations in the topic
operator's `TypeHandler`

Signed-off-by: Michael Edgar <[email protected]>
@MikeEdgar MikeEdgar force-pushed the eclipse-modifications branch from 6573c21 to cebe1bc Compare October 22, 2025 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants