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.

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

kaibocai
Copy link
Member

@kaibocai kaibocai commented Jun 23, 2023

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

FROM mcr.microsoft.com/azure-functions/java:4-java11
include above java worker PR then the end2end test should pass.

image

cc @nicktombeur

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes are added to the CHANGELOG.md
  • I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

@kaibocai kaibocai changed the title Provide a way Provide a way for customer to inject their own data converter. Jun 23, 2023
@kaibocai kaibocai requested a review from cgillum June 23, 2023 20:30
cgillum
cgillum previously approved these changes Jun 23, 2023
Copy link
Member

@cgillum cgillum left a 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);
Copy link
Member

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.

Copy link
Member Author

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);
Copy link
Member

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:

Suggested change
}, dataConverter);
}, this.dataConverter);

@ScottHamper
Copy link

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.

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