Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/1084 provide data entry sender type tx as configurable option #1086

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Feature/1084 provide data entry sender type tx as configurable option #1086

wants to merge 6 commits into from
+341 −79

Conversation

margue
Copy link

@margue margue commented Feb 13, 2025 •
edited
Loading

Basic implementation of TX-style DatapoolSender.

Details to be discussed:

  • DataEntrySender interface and use of generics needs to be checked (Taskpool uses EngineTaskCommand, Datapool uses Axon CommandMessages incl. MetaData)
  • Naming of DataEntryCommandProcessor and DataEntryCommandSender (this is the interface used by client applications, therefore potential incompatibility issue -> left unchanged in this PR)
  • CommandListGateway should be extracted to separate module, so that it can be used by datapool sender and taskpool sender (so far I copied it over to datapool sender)
  • Separate SenderProperties and TaskpoolSenderProperties from each other, so that global sender properties can be used for taskpool AND datapool (or even might be used only by new CommandListGateway module)
  • Sender-1** -> Changed log message codes to the same codes as in taskpool adding 100, so that it is clear, that all logmessages with code > 100 derive from datapool while keeping the same log message number semantics like in taskpool
  • Documentation not updated so far O:-)

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 13, 2025
View reviewed changes
...datapool-sender/src/main/kotlin/io/holunda/polyflow/datapool/DataEntrySenderConfiguration.kt Fixed Show fixed Hide fixed
...datapool-sender/src/main/kotlin/io/holunda/polyflow/datapool/DataEntrySenderConfiguration.kt Fixed Show fixed Hide fixed
...otlin/io/holunda/polyflow/datapool/sender/DirectTxAwareAccumulatingDataEntryCommandSender.kt Fixed Show fixed Hide fixed
...datapool-sender/src/main/kotlin/io/holunda/polyflow/datapool/sender/SimpleDataEntrySender.kt Fixed Show fixed Hide fixed
...main/kotlin/io/holunda/polyflow/datapool/sender/TxAwareAccumulatingDataEntryCommandSender.kt Fixed Show fixed Hide fixed
...sender/src/main/kotlin/io/holunda/polyflow/datapool/sender/gateway/AxonCommandListGateway.kt Fixed Show fixed Hide fixed
zambrovski
zambrovski requested changes Feb 13, 2025
View reviewed changes
Copy link
Member

@zambrovski zambrovski left a comment

Choose a reason for hiding this comment

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

I'll have to look on this in the IDE, not in the browser. I believe it is generally ok, but the naming of the components and extractions you proposed in the comments are worth to discuss and probably introduce some changes.

...datapool-sender/src/main/kotlin/io/holunda/polyflow/datapool/DataEntrySenderConfiguration.kt Show resolved Hide resolved
...datapool-sender/src/main/kotlin/io/holunda/polyflow/datapool/DataEntrySenderConfiguration.kt
fun configureSender(
gateway: CommandGateway,
fun dataEntryCommandProcessor(
dataEntrySender: DataEntrySender,
Copy link
Member

Choose a reason for hiding this comment

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

I believe this at least very confusing. This bean factory creates a DataEntryCommandSender and requires a DataEntrySender. Let us find better names for all this classes.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, exactly. I am not happy with the wording, as I mentioned in my PR-comments.

...datapool-sender/src/main/kotlin/io/holunda/polyflow/datapool/DataEntrySenderConfiguration.kt Outdated Show resolved Hide resolved
...ommon/datapool-sender/src/main/kotlin/io/holunda/polyflow/datapool/sender/DataEntrySender.kt
/**
* Interface for beans sending data entry commands.
*/
interface DataEntrySender {
Copy link
Member

Choose a reason for hiding this comment

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

Don't like this name. There already a DataEntryCommandSender class and this one is named to similar and provides JavaDoc saying that it indeed sends the data entry commands (so is named not according to its function).

Copy link
Author

Choose a reason for hiding this comment

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

Agreed!

...otlin/io/holunda/polyflow/datapool/sender/DirectTxAwareAccumulatingDataEntryCommandSender.kt
class DirectTxAwareAccumulatingDataEntryCommandSender(
private val commandListGateway: CommandListGateway,
dataEntrySenderProperties: DataEntrySenderProperties
) : TxAwareAccumulatingDataEntryCommandSender(
Copy link
Member

Choose a reason for hiding this comment

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

Why have you introduced the separation? Are you planing to introduce the job-based sender too (would be a mirror of the job-based sender for tasks)?

Copy link
Author

Choose a reason for hiding this comment

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

Might be a valid next step, I thought of. And also keeping the structure consistent to Taskpool Sender.

@zambrovski
Copy link
Member

And please double check, why Codacy tells you that you introduced 6 new issues.

@zambrovski zambrovski added this to the 4.3.1 milestone Feb 13, 2025
@zambrovski zambrovski linked an issue Feb 13, 2025 that may be closed by this pull request
Provide DataEntrySenderType 'tx' as configurable option (similar to SenderType 'tx' in TaskpoolSender) #1084
Open
@margue
fix(holunda-io#1084):
8624640 
- adds missing docs to DataEntrySenderConfiguration
- declares simpleDataEntryCommandSender Bean as DataEntrySender
@margue
fix(holunda-io#1084): adds missing docs to DirectTxAwareAccumulatingD…
0294a10 
…ataEntryCommandSender
@margue
fix(holunda-io#1084): adds missing docs for companion objects
5d378c9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zambrovski zambrovski zambrovski requested changes

Assignees

@margue margue

Labels
None yet
Projects
None yet
Milestone
4.3.1
Development

Successfully merging this pull request may close these issues.

Provide DataEntrySenderType 'tx' as configurable option (similar to SenderType 'tx' in TaskpoolSender)
2 participants
@margue @zambrovski