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

Change to remote ejb server implementation #175

Merged
merged 10 commits into from
Dec 13, 2016

Conversation

tadamski
Copy link
Contributor

@tadamski tadamski commented Dec 5, 2016

This commit is a part of integrating RemoteSever with ejb3 subsystem.

@@ -288,10 +290,10 @@ public void processInvocation(final EJBReceiverInvocationContext receiverContext
}

// write sec context
marshaller.writeInt(peerIdentityId);
marshaller.writeShort(peerIdentityId);
Copy link
Member

Choose a reason for hiding this comment

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

These have to be 4 byte integers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed on client and server side.

@@ -539,7 +546,7 @@ public void writeResponse(final Object response) {
}
}

public void writeException(final Exception response) {
public void writeThrowable(final Throwable response) {
Copy link
Member

Choose a reason for hiding this comment

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

We don't ever want to write Errors or anything like that; generally the EJB protocol and spec deal in Exceptions only. Was there some reason this had to be widened?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current org.jboss.as.ejb3.remote.protocol.versionone.SessionOpenRequestHandler catches Throwable during session id generation so I assumed that some Errors may be thrown during session id generation.

@@ -308,14 +303,20 @@ void handleInvocationRequest(final int invId, final InputStream input) throws IO
int transactionId = unmarshaller.readUnsignedShort();
identity = identityId == 0 ? connection.getLocalIdentity() : connection.getLocalIdentity(identityId);
transaction = null; // todo: look up transaction by transactionId
locator = unmarshaller.readObject(EJBLocator.class);
Copy link
Member

Choose a reason for hiding this comment

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

Moving these methods changes the protocol (see protocol.txt). I think the app/mod name should come first, as described in that doc. Did you encounter a reason to change it?

receiverContext.resultReady(new EJBReceiverInvocationContext.ResultProducer.Failed(e));
return;
} else {
//TODO Elytron
Copy link
Member

Choose a reason for hiding this comment

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

This should be functional in the latest Remoting/Elytron versions. Did you encounter a problem?

Copy link
Contributor Author

@tadamski tadamski Dec 5, 2016

Choose a reason for hiding this comment

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

Encountered NullPointerException - checking versions... The same problem with current HEADs of remoting and elytron.

@@ -416,7 +416,7 @@ private void writeFailedResponse(final int invId, final Exception e) {
this.transaction = transaction;
}

int getInvId() {
public int getInvId() {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this public?

Copy link
Contributor Author

@tadamski tadamski Dec 5, 2016

Choose a reason for hiding this comment

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

This is used in Association class implementation to trace incomming invocations on the server side.

@wildfly-ci
Copy link

Build 224 outcome was FAILURE using a merge of 64f9bdb
Summary: Exit code 1 (new) Build time: 00:00:03

@tadamski
Copy link
Contributor Author

Failure happens because I have made the version of jboss-remoting 5.0.0.Beta13 - will retest after the release.

@dmlloyd
Copy link
Member

dmlloyd commented Dec 13, 2016

I've released 5.0.0.Beta13 of Remoting. Once that's updated in the POM I will merge this.

@tadamski
Copy link
Contributor Author

I have already made it 5.0.0.Beta13 in the PR.
Retest this please

@dmlloyd
Copy link
Member

dmlloyd commented Dec 13, 2016

Retest this please

@wildfly-ci
Copy link

Build 225 outcome was FAILURE using a merge of 64f9bdb
Summary: Exit code 1 Build time: 00:00:09

@dmlloyd
Copy link
Member

dmlloyd commented Dec 13, 2016

I manually restarted the test, looks like CI has cached the non-existence of this release. I'll try it again.

@wildfly-ci
Copy link

Build 226 outcome was FAILURE using a merge of 64f9bdb
Summary: Exit code 1 Build time: 00:00:03

@wildfly-ci
Copy link

Build 227 outcome was FAILURE using a merge of 64f9bdb
Summary: Exit code 1 Build time: 00:00:02

@dmlloyd dmlloyd merged commit ee10d8c into wildfly:master Dec 13, 2016
@tadamski tadamski deleted the remote_server branch July 6, 2022 12:18
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