Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Allow Overrding of IDGenerator #2910

Merged
merged 9 commits into from
Apr 25, 2022
Merged

Conversation

v1r3n
Copy link
Contributor

@v1r3n v1r3n commented Apr 10, 2022

Pull Request type

  • Bugfix
  • [ x] Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes (Please run ./gradlew generateLock saveLock to refresh dependencies)
  • WHOSUSING.md
  • Other (please describe):

Changes in this PR

IDGenerator is a fixed class today with a static generate() method to generate ID for workflows and tasks.
This makes it impossible to override the id generation logic for cases where users want to have a separate scheme for id generation.

This change allows users to override the IDGenerator class while keeping the default implementation intact.

Describe the new behavior from this PR, and why it's needed
Issue #

Alternatives considered

Describe alternative implementation you have considered

@jxu-nflx
Copy link
Contributor

Hello @v1r3n currently the Cassandra module requires an uuid for workflow and tasks: https://github.com/Netflix/conductor/blob/main/cassandra-persistence/src/main/java/com/netflix/conductor/cassandra/dao/CassandraBaseDAO.java#L152
So if a custom ID generator is used, user needs to be aware of that might not work with Cassandra module. Could you please add comment or document it somewhere?

Also, the id generator is all over the code bases, would you be interested to refactor that so to move everything to the storage layer?

@v1r3n
Copy link
Contributor Author

v1r3n commented Apr 21, 2022

Hello @v1r3n currently the Cassandra module requires an uuid for workflow and tasks: https://github.com/Netflix/conductor/blob/main/cassandra-persistence/src/main/java/com/netflix/conductor/cassandra/dao/CassandraBaseDAO.java#L152 So if a custom ID generator is used, user needs to be aware of that might not work with Cassandra module. Could you please add comment or document it somewhere?

I have added a note on the Id Generator class to indicate that C* needs IDs to be valid UUIDs and the overriding should be done with careful consideration on the storage layer.

Also, the id generator is all over the code bases, would you be interested to refactor that so to move everything to the storage layer?

Happy to do this, I did consider this earlier, but that will require significant refactoring of the code - I can send another PR after this one is merged so we can break this down in multiple pushes.

@jxu-nflx jxu-nflx merged commit 3a03815 into Netflix:main Apr 25, 2022
@v1r3n v1r3n deleted the id_generator_override branch June 15, 2022 06:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants