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

ISPN-6893 Scala removal from server/core module #4449

Closed
wants to merge 1 commit into from

Conversation

wburns
Copy link
Member

@wburns wburns commented Jul 8, 2016

No description provided.

@wburns wburns added the Preview label Jul 8, 2016
@slaskawi
Copy link
Contributor

The code looks really good... awesome amount of work!

Just a heads up - it will collide with Muti-tenancy PR: #4348 but hopefully not that much.

Also those test failures seem to be related.

}

private ClientAuth requireClientAuth(SslConfiguration sslConfig) {
return sslConfig.enabled() ? ClientAuth.REQUIRE : ClientAuth.NONE;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be sslConfig.requireClientAuth() ;)

TL;DR
After turning on SSL Debug it turned out that server challenges client for certificate (which was not associated with the client keystore). This suggested that either client cert file is wrong or server challenge is wrong. I was lucky with the latter :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha, thanks :) - I figured it was in NettyChannelInitializer somewhere, not sure how I didn't see this one. Thanks!

@wburns wburns force-pushed the scala_removal branch 2 times, most recently from 7c50a49 to 50130f1 Compare July 21, 2016 21:10
@slaskawi
Copy link
Contributor

You could probably squash this one with #4468. After that you will probably win "the biggest commit" contest ;) Even though there is a strong competitor: #4348

@wburns
Copy link
Member Author

wburns commented Jul 26, 2016

I am trying to leave the commits separated by module so it is easier to track the changes. I will be adding JIRAs for them today.

@wburns wburns changed the title [PREVIEW] Scala removal from server/core module ISPN-6893 Scala removal from server/core module Jul 26, 2016
@wburns
Copy link
Member Author

wburns commented Jul 26, 2016

Ready for review/integration.

public abstract class AbstractProtocolServer<A extends ProtocolServerConfiguration> extends AbstractCacheIgnoreAware
implements ProtocolServer<A> {

private final JavaLog log = LogFactory.getLog(getClass(), JavaLog.class);
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to rename "JavaLog" to just "Log" like in all other modules ?

Copy link
Member Author

Choose a reason for hiding this comment

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

That was the name before, I can change it.

import java.util.Set;

/**
* @author gustavonalle
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add something meaningful to this javadoc?

@slaskawi
Copy link
Contributor

Awesome work! Some small, really picky stuff found.

@galderz
Copy link
Member

galderz commented Aug 2, 2016

Looks good! Bye bye scala 😢 😉

Needs rebasing though.

@wburns
Copy link
Member Author

wburns commented Aug 2, 2016

Rebased, will fix up others.

@wburns
Copy link
Member Author

wburns commented Aug 2, 2016

Updated, just wait for a test run to verify nothing is broken before integrating.

@tristantarrant
Copy link
Member

Pulling !

@tristantarrant
Copy link
Member

Pushed, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants