-
Notifications
You must be signed in to change notification settings - Fork 7
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.
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
Provide a way for customer to inject their own data converter. #145
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
throw new IllegalStateException("Multiple implementations of DataConverter found on the classpath."); | ||
} | ||
} | ||
oneTimeLogicExecuted.compareAndSet(false,true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious why you can't use a regular Boolean
here? Since we're already in a synchronized
block, I wouldn't expect that you'd need to use any additional synchronization primitives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I use a regular Boolean
in java, I need to declare it with volatile
which avoids using thread cache, that means that when a thread modifies the value of this Boolean
, all other threads will see the updated value when they access it in Java. So I am thinking of just using AtomicBoolean which provides both visibility and atomicity at the same time.
throw (OrchestratorBlockedException) cause; | ||
} | ||
throw new RuntimeException("Unexpected failure in the task execution", e); | ||
} | ||
}); | ||
}, dataConverter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make it clear this is a member variable and not a local variable:
}, dataConverter); | |
}, this.dataConverter); | |
ebb3786
to
0994c8c
Compare
Hey All, Any chance this feature can get merged soon? I've been experimenting with Azure Durable Functions using Kotlin, and desperately wish I could use kotlinx serialization instead of Jackson. |
Issue describing the changes in this PR
Try to improve #134. This PR provides the customer a way to provide their own data converter through java SPI. To have it work on java11 and above, the dependent PR Azure/azure-functions-java-worker#717 has to be released in java worker.
Have to do more tests on this PR. Will try to provide more details once the PR is ready.Tested on local worked as expected.
Right now for the issue mentioned in #134, using my customized data converter based on gson works fine
The end2end test failed because the java worker is not released yet, this feature shall wait for the PR at https://github.com/Azure/azure-functions-java-worker/pull/717/files to be released first.
Once the image here
durabletask-java/samples-azure-functions/Dockerfile
Line 1 in 57133bd
cc @nicktombeur
Pull request checklist
CHANGELOG.md
Additional information
Additional PR information