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

PAYARA-3654 Client-side adapter support for EJB HTTP Client #3931

Merged
merged 9 commits into from May 14, 2019

Conversation

Projects
None yet
4 participants
@pdudits
Copy link
Contributor

commented May 3, 2019

This implements the proposed solution of adding client side API for creating adapters - application specific proxies, that emulate stateful behavior, and in the end delegate to a stateless remote call.

The PR is best reviewed commit by commit:

38fde3a defines the API, javadoc and some basic tests
967d8b0 is a clenup of previous api
26fc84e demonstrates how the adapter can be used for emulating JMS api on client side, where remote part implements single method sendTextMessageToQueue

pdudits added some commits May 3, 2019

PAYARA-3654: Clean up API
Give factory properties more descriptive names
Remove all non-public classes from deprecated package
@pdudits

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

jenkins test please

@pdudits

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

jenkins test please

@pdudits pdudits requested review from fturizo and arjantijms May 9, 2019

@pdudits pdudits requested a review from jbee May 13, 2019

@jbee
Copy link
Contributor

left a comment

Did not look at all details since I would like to see some refinement of the adapter mechanism first.

pdudits added some commits May 13, 2019

PAYARA-3654: Rename makeClientAdapter to makeLocalProxy
Signed-off-by: Patrik Dudits <patrik.dudits@payara.fish>
PAYARA-3654: Rename ClientAdapterRegistry to CompositeClientAdapter
Signed-off-by: Patrik Dudits <patrik.dudits@payara.fish>
@jbee
Copy link
Contributor

left a comment

Looking at it second time with better understanding I had some more thoughts. Not necessarily change requests but there might be some changes coming out of it.

* @return new customizer instance
*/
public ClientAdapterCustomizer transformName(Function<String, String> nameTransformation) {
ClientAdapterCustomizer conditional = new ClientAdapterCustomizer(this);

This comment has been minimized.

Copy link
@jbee

jbee May 13, 2019

Contributor

null can be set but is not handled later as default of identity is assumed.

This comment has been minimized.

Copy link
@pdudits

pdudits May 13, 2019

Author Contributor

Ok, will use identity when null is provided

@Override
public Optional<Object> makeLocalProxy(String jndiName, Context remoteContext) throws NamingException {
Optional<ResolutionResult> resolutionResult = adapterSuppliers.stream()
.map(Supplier::get)

This comment has been minimized.

Copy link
@jbee

jbee May 13, 2019

Contributor

Now came to think if it makes much sense to call Supplier#get for each invocation of the method. This might create multiple adapters where only one was expected or meant to be created.

This comment has been minimized.

Copy link
@pdudits

pdudits May 13, 2019

Author Contributor

I realized using suppliers is too meta in this context. ClientAdapter is already a factory, so we shouldn't need factory factory for it.

.map(Supplier::get)
.map(adapter -> resolve(adapter, jndiName, remoteContext))
.filter(Optional::isPresent)
.findFirst()

This comment has been minimized.

Copy link
@jbee

jbee May 13, 2019

Contributor

For consideration: As we use the first present and results throwing a NamingException become a present optional creating the extra type just to do this stream acrobatic seems overly complicated to me. With a loop normal throwing and catching could be used which I think is more readable in this case.

This comment has been minimized.

Copy link
@pdudits

pdudits May 13, 2019

Author Contributor

Yep, you're right.

Show resolved Hide resolved ...ain/java/fish/payara/ejb/http/client/adapter/CompositeClientAdapter.java
Show resolved Hide resolved ...t/src/main/java/fish/payara/ejb/rest/client/RemoteEJBContextFactory.java
Show resolved Hide resolved ...t/java/fish/payara/ejb/http/client/adapter/example/stub/NoOpJmsStub.java
PAYARA-3654: Drop support for Supplier<ClientAdapter>, simplify logic
ClientAdapter is already a factory, so supporting factory factory is
unnecessary.

Replacing streams with simple for loop simplifies lookup logic.

Signed-off-by: Patrik Dudits <patrik.dudits@payara.fish>
@jbee

jbee approved these changes May 13, 2019

Copy link
Contributor

left a comment

Looks good now :)

@pdudits

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

jenkins test please

PAYARA-3654: Remove tests for removed feature
Signed-off-by: Patrik Dudits <patrik.dudits@payara.fish>
@pdudits

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

jenkins test please

@pdudits pdudits merged commit 095078c into payara:master May 14, 2019

59 checks passed

Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/javaee-api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details

@pdudits pdudits deleted the pdudits:payara-3654 branch May 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.