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

Some fixes for running cometd6 on jetty-10 #3702

Merged
merged 4 commits into from
May 28, 2019

Conversation

gregw
Copy link
Contributor

@gregw gregw commented May 28, 2019

some minor fixes found while testing cometd6

{
super.init(config);
config.getServletContext().log("org.eclipse.jetty.websocket.servlet.WebSocketUpgradeFilter is deprecated. Use org.eclipse.jetty.websocket.server.WebSocketUpgradeFilter");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The message is wrong, it should be the other way around.
Use WebSocketUpgradeFilter.class.getName() instead of the explicit string.
Also, you want this to be logged via a normal Logger instance too, not only using the ServletContext.log?

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 the message.
The context logger is typically just a normal Logger also, so it get's the message out.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw force-pushed the jetty-10.0.x-cometd6-fixes branch from ec7b268 to da34fba Compare May 28, 2019 10:13
Signed-off-by: Greg Wilkins <gregw@webtide.com>
import javax.servlet.ServletException;

/**
* @Deprecated Moved to #org.eclipse.jetty.websocket.servlet.WebSocketUpgradeFilter
Copy link
Contributor

Choose a reason for hiding this comment

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

The javadoc deprecation as lower case d for "deprecated".

super.init(config);
config.getServletContext().log(
WebSocketUpgradeFilter.class.getName() +
" is deprecated Use " +
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a comma or a full stop? Capital U for "Use"?

@gregw gregw requested a review from sbordet May 28, 2019 11:58
gregw added 2 commits May 28, 2019 15:28
Signed-off-by: Greg Wilkins <gregw@webtide.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

A minor other nit, otherwise looks good.

if (_dispatches==0)
return type==REQUEST || type==ASYNC && _holder.isAsyncSupported();
return type==REQUEST || type==ASYNC && holder.isAsyncSupported();
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit pick, can you properly indent this return?

@gregw gregw merged commit 3e0e6a7 into jetty-10.0.x May 28, 2019
@gregw gregw deleted the jetty-10.0.x-cometd6-fixes branch May 29, 2019 05:29
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