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-3864 EJB Java serialisation alongside with JSONB #4037

Merged
merged 36 commits into from Jul 4, 2019

Conversation

@jbee
Copy link
Contributor

commented Jun 11, 2019

Description

Adds implementation for https://payara.atlassian.net/wiki/spaces/PAYAR/pages/851836929/EJB+HTTP+Protocol+version+1
including both Java serialisation and JSONB as well as automatic detection of served protocol by the client. In case both versions are supported the client chooses the newer version.

To explicitly select a serialisation type or protocol version two environment variables were added to the RemoteEJBContextFactory:

  • fish.payara.serialization given as "java" or "json" (default) - not case sensitive
  • fish.payara.version given as "0" or "1" (default)

Testing performed

Component tests were added to both the endpoint and the client.
The existing integration test were adopted payara/payara-samples#17 so they also cover the new protocol version for both Java Serialisation and JSONB.

@jbee jbee added this to the 5.193 milestone Jun 11, 2019

@jbee jbee self-assigned this Jun 11, 2019

@jbee

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

jenkins test please

@@ -86,6 +86,36 @@
<version>${project.version}</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>commons-io</groupId>

This comment has been minimized.

Copy link
@smillidge

smillidge Jun 11, 2019

Contributor

Anyway this can be done without adding the commons dependency?

This comment has been minimized.

Copy link
@jbee

jbee Jun 11, 2019

Author Contributor

I use if for the ClassLoaderObjectInputStream. Its Apache licensed so I guess we should be allowed to only grab that class?

We already do use commons-io elsewhere so if we don't want to use it we should probably remove it everywhere? Maybe you can help me understand why this dependency is unwanted while we have lots of upstream dependencies. Is it because its a general utility that might cause trouble in case applications use it in a different version?

This comment has been minimized.

Copy link
@smillidge

smillidge Jun 11, 2019

Contributor

If we are using it anyway then I'm fine with it.

This comment has been minimized.

Copy link
@smillidge

smillidge Jun 11, 2019

Contributor

In general there are a number of reasons we don't want to bring in new 3rd party dependencies lightly;

  1. It bloats the server
  2. Applications may use a different version and therefore it can cause problems
  3. It could increase the attack surface of the server.
  4. They need a license review and update to all the 3rd party license files

This comment has been minimized.

Copy link
@jbee

jbee Jun 12, 2019

Author Contributor

As discussed on Teams we avoid adding commons.io by repackaging the needed class.

This comment has been minimized.

Copy link
@jbee

jbee Jun 18, 2019

Author Contributor

Backed adding as repackaged class since removing commons.io from nucleus results in errors. It is needed. Created PAYARA-3923 to investigate where it is used and if we could remove it.

@smillidge
Copy link
Contributor

left a comment

Can you remove commons as we should avoid bringing in 3rd party dependencies as much as possible.

@jbee jbee force-pushed the jbee:PAYARA-3864-ejb-java-serialise branch from a8df4c1 to 6004e78 Jun 12, 2019

@jbee

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

jenkins test please

pdudits and others added 12 commits Jun 12, 2019
Merge pull request #2 from pdudits/payara-3864-client
Client-side protocol discovery
@jbee

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

jenkins test please (what to see if there is another build problem)

pdudits and others added 7 commits Jun 17, 2019
@jbee

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

jenkins test please

@jbee

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

jenkins test please

@Pandrex247
Copy link
Member

left a comment

Just a few minor niggles

ClientBuilder clientBuilder = null;
try {
clientBuilder = getClientBuilder();
Client client = clientBuilder.build();

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Jun 28, 2019

Member

Could you not have just put this on one line?

Client client = clientBuilder.build();

LookupDiscoveryServiceImpl lookupDiscovery = new LookupDiscoveryServiceImpl();
LookupDiscoveryResponse discovery = lookupDiscovery.discover(client, root);

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Jun 28, 2019

Member

Again, could have been on one line.

@@ -79,7 +79,8 @@
public static final String JAXRS_CLIENT_HOSTNAME_VERIFIER = "fish.payara.hostnameVerifier";
public static final String JAXRS_CLIENT_EXECUTOR_SERVICE = "fish.payara.executorService";
public static final String JAXRS_CLIENT_CONNECT_TIMEOUT = "fish.payara.connectTimeout";

public static final String JAXRS_CLIENT_SERIALIZATION = "fish.payara.serialization";
public static final String JAXRS_CLIENT_VERSION = "fish.payara.version";

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Jun 28, 2019

Member

I think both of these probably need to be a bit more specific.
fish.payara.version particularly sounds more like a property to get the version of Payara itself rather than the version of the JAX-RS client.

This comment has been minimized.

Copy link
@pdudits

pdudits Jun 28, 2019

Contributor

I agree on that one, also JAXRS_CLIENT_VERSION seems to be telling different story. Probably it should mention protocol version

This comment has been minimized.

Copy link
@jbee

jbee Jun 29, 2019

Author Contributor

I thought the same myself but the existing ones didn't use some namespacing so I did not add one. In hindsight it might have been nicer to have a common namespace like fish.payara.ejb.http for all of them. Any particular suggestions on the two names?

This comment has been minimized.

Copy link
@Pandrex247

Pandrex247 Jul 2, 2019

Member

Actually I completely overlooked those other properties - yeah it probably would be better to have a proper namespace for them.

TBH I like the namespace you gave, fish.payara.ejb.http, but I recognise that it's perhaps a bit verbose. I think the names are fine if you use that namespace though (e.g. fish.payara.ejb.http.version).

I kind of agree with Patrik in that the variable name for JAXRS_CLIENT_VERSION is a bit misleading as it seems to be more to do with the EJB HTTP protocol version (unless I'm misreading it).

This comment has been minimized.

Copy link
@jbee

jbee Jul 2, 2019

Author Contributor

Changed the name and properties.

@jbee

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

jenkins test please

Changes made

@Pandrex247 Pandrex247 merged commit 9abf9ce into payara:master Jul 4, 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
@jbee

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Thanks @Pandrex247

AlanRoth pushed a commit to AlanRoth/Payara that referenced this pull request Aug 13, 2019
Updated versions of dependencies to just released GF 5.1.0-RC2 (payar…
…a#4037)

* Updated dependencies on GF 5.1.0-RC2

Signed-off-by: Jan Supol <jan.supol@oracle.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.