feat: ResourceIDMapper for external event sources, external dependents and bulk dependents #3020
Conversation
docs/content/fileList.txt
Outdated
There was a problem hiding this comment.
This is a deletion of empty file, it is not needed.
...or-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDMapper.java
Outdated
Show resolved
Hide resolved
...or-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDMapper.java
Outdated
Show resolved
Hide resolved
...or-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDMapper.java
Show resolved
Hide resolved
...-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDProvider.java
Outdated
Show resolved
Hide resolved
...-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDProvider.java
Outdated
Show resolved
Hide resolved
...-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDProvider.java
Outdated
Show resolved
Hide resolved
...-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ResourceIDProvider.java
Show resolved
Hide resolved
xstefank
left a comment
xstefank
left a comment
There was a problem hiding this comment.
Small stuff, otherwise good
docs/content/en/docs/documentation/dependent-resource-and-workflows/dependent-resources.md
Outdated
Show resolved
Hide resolved
metacosm
left a comment
metacosm
left a comment
There was a problem hiding this comment.
Several comments:
- As previously mentioned, I don't think that putting the responsibility of providing the external resource id on the external resource class is a good idea. We need to decide which design to go with now because this is already a breaking change and it shouldn't change again soon after, if we decide that putting the id "generation" on the dependent class (for example) is a better design later.
- The documentation for this feature is rather lacking and doesn't really explain the concepts or how to use things so it needs to be improved, imo.
That is completely optional, but allows us to add meaningful default implementation for the mapper. What alternative would you propose?
Not shure what do you mean by this. Can you pls elaborate?
Yeah, in general we have quite badly documented, features around it, would rather followup with a comprehensive rework on the docs. |
| */ | ||
| static <R> ResourceIDMapper<R, String> singleResourceResourceIDMapper() { | ||
| return r -> "josdk:mapper:id"; | ||
| return r -> "id"; |
There was a problem hiding this comment.
The value should be irrelevant and we should rather use a value that has not risk of conflicting with an identifier a user would create.
There was a problem hiding this comment.
Will have to upgrade upgrade the javadocs, there is no situation where there can be a collision. Assuming that this is by definition not usable for BulkDependentResources - will add javadoc.
If you see some situation where it actually can lead to a collision pls describe it.
There was a problem hiding this comment.
What's the harm of choosing a value that we're sure won't cause a collision, regardless of whether one such collision can happen? I mean if the value doesn't matter, then why not choose one that doesn't look like it is meaningful? For that matter, it could be a totally random one.
There was a problem hiding this comment.
I belive in this cases we should focus:
- the readability and simplicity is the key;
- And of course we should not solve not solve issues which are not present - so if there is no situation where it can cause collision, then we should not solve the issue of collision; since that would cause confusion.
So to fulfill those two as you said, any string that is simple enough would do. I choose simply id, but could be for example void, anyId, any string. But imho (and maybe this is a personal pref in some cases), should not be any string that if someone reads the code will have to start thinking why is that a "special value".
There was a problem hiding this comment.
But added also a comment that it could be any values, feel free to choose any simple and expressive string literal, it is fine by me.
…pper Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Will check if this could be elegantly extended to bulk resources Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
…s and bulk dependents Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
…tor/processing/ResourceIDMapper.java Co-authored-by: Martin Stefanko <xstefank122@gmail.com>
…tor/processing/ResourceIDMapper.java Co-authored-by: Martin Stefanko <xstefank122@gmail.com>
…tor/processing/ResourceIDProvider.java Co-authored-by: Martin Stefanko <xstefank122@gmail.com>
…tor/processing/ResourceIDProvider.java Co-authored-by: Martin Stefanko <xstefank122@gmail.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Chris Laprun <metacosm@gmail.com>
it is by definition single, so namespace scoping is out of the picture, also does not confuse this way with some hidden intention in the background Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
Signed-off-by: Attila Mészáros <a_meszaros@apple.com>
csviri
force-pushed
the
resource-id-mapper-bulk
branch
from
adeabcc to
100a09d
Compare
No description provided.