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

#476 Make MGCP channel buffer size configurable. #700

Merged
merged 5 commits into from Dec 19, 2017

Conversation

Projects
None yet
3 participants
@ibrarahmad
Contributor

ibrarahmad commented Nov 27, 2017

A new configuration parameter added in mediaserver.xml file called mgcpBuffer's size. The default value for that variable set to 5000.

#476 Make MGCP channel buffer size configurable.
    A new configuration parameter added in mediaserver.xml file called
    mgcpBuffer's size. The default value for that variable set to 5000.
@gsaslis

Thanks for the PR @ibrarahmad !
Please take a look at some comments i've made below.

@hrosa the floor is all yours! ;)

private static final ChannelHandler[] NO_HANDLERS = new ChannelHandler[0];
private final ChannelHandler[] handlers;
public MgcpChannelInitializer(ChannelHandler... handlers) {
this.handlers = (handlers == null || handlers.length == 0) ? NO_HANDLERS : handlers;
public MgcpChannelInitializer(int mgcpBufferSize, ChannelHandler... handlers) {

This comment has been minimized.

@gsaslis

gsaslis Nov 28, 2017

Contributor

Please try to see if you can apply the same change without breaking backwards compatibility.

(this is a public constructor, so by directly changing its signature you're introducing a non-backwards compatible change: anyone using the old constructor must change their code).

This comment has been minimized.

@ibrarahmad

ibrarahmad Nov 28, 2017

Contributor

I have created new constructor to avoid backward compatibility breakage.

@Inject
public MgcpChannelInitializerProvider(@Named("mgcpNetworkGuard") NetworkGuard networkGuard, MgcpMessageParser parser, MgcpChannelInboundHandler inboundHandler) {
public MgcpChannelInitializerProvider(MediaServerConfiguration config, @Named("mgcpNetworkGuard") NetworkGuard networkGuard, MgcpMessageParser parser, MgcpChannelInboundHandler inboundHandler) {

This comment has been minimized.

@gsaslis

gsaslis Nov 28, 2017

Contributor

same comment re: backwards-incompatible changes

public void setMgcpBufferSize(int mgcpBufferSize) {
if (mgcpBufferSize < 2000 || mgcpBufferSize > 10000) {
throw new IllegalArgumentException("Illegal MGCP buffer size value: 2000 < " + port + " < 10000");

This comment has been minimized.

@gsaslis

gsaslis Nov 28, 2017

Contributor

very minor one here, but you probably want to try to reformat this. Any time the exception is thrown you'll get some odd looking output (the port value will never be in the range).

This comment has been minimized.

@ibrarahmad

ibrarahmad Nov 28, 2017

Contributor

Oh sorry for typo, fixed in new commit.

#476 Make MGCP channel buffer size configurable.
    Added new constructor for MgcpChannelInitializerProvider and
    MgcpChannelInitializer classes for backward compatibility.
@hrosa

Hi @ibrarahmad thanks for the contribution, requested some very small changes.

As part of the ticket, the documentation must be updated to include explanation about the new parameter: https://github.com/RestComm/mediaserver/blob/master/docs/sources-asciidoc/src/main/asciidoc/concept-chapter-Configuring_the_Media_Server.adoc#controller-configuration

Also, we have a small set of scripts for automatic configuration that must be updated as well, to include support for new parameter.

Add new parameter here: https://github.com/RestComm/mediaserver/blob/master/bootstrap/src/main/config/autoconfig/mediaserver.conf#L8

Then implement method to persist the config here:
https://github.com/RestComm/mediaserver/blob/master/bootstrap/src/main/config/autoconfig/config-controller-mgcp.sh#L5

Finally, make sure that the changes you introduce as part of your development work are reflected in a test case. You can update this test case:

https://github.com/RestComm/mediaserver/blob/master/bootstrap/src/test/java/org/restcomm/media/bootstrap/configuration/XmlConfigurationLoaderTest.java#L54

I think that is all. Once again, many thanks!

Best Regards

this.mgcpBufferSize = 5000;
}
public MgcpChannelInitializerProvider(MediaServerConfiguration config, @Named("mgcpNetworkGuard") NetworkGuard networkGuard, MgcpMessageParser parser, MgcpChannelInboundHandler inboundHandler) {

This comment has been minimized.

@hrosa

hrosa Dec 1, 2017

Collaborator

Please remove this constructor. This is a Guice Provider which relies on dependency injection, so only one constructor is accepted.

On a side note, @gsaslis comments were valid, try to follow them as much as possible. This is an exceptional case.

@@ -43,18 +45,31 @@
private final MgcpMessageDecoder decoder;
private final MgcpMessageEncoder encoder;
private final MgcpChannelInboundHandler inboundHandler;
private final MgcpControllerConfiguration controller;

This comment has been minimized.

@hrosa

hrosa Dec 1, 2017

Collaborator

Either store configuration POJO or mgcpBufferSize. There's no point keeping references to both.

@Inject
public MgcpChannelInitializerProvider(@Named("mgcpNetworkGuard") NetworkGuard networkGuard, MgcpMessageParser parser, MgcpChannelInboundHandler inboundHandler) {
this.filter = new NetworkFilter(networkGuard);
this.decoder = new MgcpMessageDecoder(parser);
this.encoder = new MgcpMessageEncoder();
this.inboundHandler = inboundHandler;
this.controller = null;

This comment has been minimized.

@hrosa

hrosa Dec 1, 2017

Collaborator

Pass configuration POJO as parameter in constructor and assign here.

@Inject
public MgcpChannelInitializerProvider(@Named("mgcpNetworkGuard") NetworkGuard networkGuard, MgcpMessageParser parser, MgcpChannelInboundHandler inboundHandler) {
this.filter = new NetworkFilter(networkGuard);
this.decoder = new MgcpMessageDecoder(parser);
this.encoder = new MgcpMessageEncoder();
this.inboundHandler = inboundHandler;
this.controller = null;
this.mgcpBufferSize = 5000;

This comment has been minimized.

@hrosa

hrosa Dec 1, 2017

Collaborator

The configuration POJO is responsible for setting the default value, not the Guice provider.

private static final ChannelHandler[] NO_HANDLERS = new ChannelHandler[0];
private final ChannelHandler[] handlers;
public MgcpChannelInitializer(ChannelHandler... handlers) {
this.handlers = (handlers == null || handlers.length == 0) ? NO_HANDLERS : handlers;
this.mgcpBufferSize = 5000;

This comment has been minimized.

@hrosa

hrosa Dec 1, 2017

Collaborator

magic number, create a constant default value.

This comment has been minimized.

@hrosa

hrosa Dec 1, 2017

Collaborator

code duplication, invoke MgcpChannelInitializer(int mgcpBufferSize, ChannelHandler... handlers) instead

}
public void setMgcpBufferSize(int mgcpBufferSize) {
if (mgcpBufferSize < 2000 || mgcpBufferSize > 10000) {

This comment has been minimized.

@hrosa

hrosa Dec 1, 2017

Collaborator

what was the criteria for these limits?

This comment has been minimized.

@ibrarahmad

ibrarahmad Dec 4, 2017

Contributor

I don't have any specific reason for limit. Do you have an idea about the limit, if not let me study that in detail.

This comment has been minimized.

@hrosa

hrosa Dec 7, 2017

Collaborator

just make sure it's a positive number, I guess it's up to user to decide.

@@ -21,6 +21,7 @@
<controller protocol="mgcp">
<address>127.0.0.1</address>
<port>2427</port>
<mgcpBuffer size="5000" />

This comment has been minimized.

@hrosa

hrosa Dec 1, 2017

Collaborator

use <bufferSize>5000</bufferSize> instead

This comment has been minimized.

@ibrarahmad

ibrarahmad Dec 4, 2017

Contributor

Is "bufferSize" not a general name? What about some kind of specific name which is easy to refer in documentation.

This comment has been minimized.

@hrosa

hrosa Dec 7, 2017

Collaborator

I guess you have a point, we may call it channelBuffer for example

@@ -100,6 +100,7 @@ private static void configureController(HierarchicalConfiguration<ImmutableNode>
// Basic Controller configuration
dst.setAddress(src.getString("address", MgcpControllerConfiguration.ADDRESS));
dst.setPort(src.getInt("port", MgcpControllerConfiguration.PORT));
dst.setMgcpBufferSize(src.getInt("mgcpBuffer[@size]", MgcpControllerConfiguration.MGCP_BUFFER_SIZE));

This comment has been minimized.

@hrosa

hrosa Dec 1, 2017

Collaborator

use bufferSize instead of mgcpBuffer[@size]

@hrosa hrosa self-assigned this Dec 1, 2017

@ibrarahmad

This comment has been minimized.

Contributor

ibrarahmad commented Dec 4, 2017

Thanks @hrosa for the review, I was really looking for the documentation and testing link. I will update the PR and send it you.

#476 Make MGCP channel buffer size configurable.
    Change buffer name to channelBuffer. Update test case, configuration
    script and documentation.
@ibrarahmad

This comment has been minimized.

Contributor

ibrarahmad commented Dec 13, 2017

@hrosa please have a look at the changes.

@@ -87,6 +89,8 @@ The *Port* parameter defines the port where the control channel will listen to f
The *PoolSize* parameter defines the number of MGCP requests that Media Server can handle concurrently.
The *channelBuffer* parameter definses the MGCP channel buffer size. By default it is set to 5000.

This comment has been minimized.

@hrosa

hrosa Dec 15, 2017

Collaborator

Typo definses should read defines. Explain it limits the size of incoming MGCP messages that are received by the MGCP channel.

This comment has been minimized.

@ibrarahmad

ibrarahmad Dec 15, 2017

Contributor

Done

private String address;
private int port;
private int mgcpBufferSize = MGCP_BUFFER_SIZE;
private int channelBuffer = CHANNEL_BUFFER;

This comment has been minimized.

@hrosa

hrosa Dec 15, 2017

Collaborator

already initialised in default constructor.

This comment has been minimized.

@ibrarahmad

ibrarahmad Dec 15, 2017

Contributor

Done

@@ -46,30 +46,21 @@
private final MgcpMessageEncoder encoder;
private final MgcpChannelInboundHandler inboundHandler;
private final MgcpControllerConfiguration controller;

This comment has been minimized.

@hrosa

hrosa Dec 15, 2017

Collaborator

do we need to keep reference to this configuration object?

This comment has been minimized.

@ibrarahmad

ibrarahmad Dec 15, 2017

Contributor

I don't think so there is any harm to do that

#476 Make MGCP channel buffer size configurable.
    Fix a typo in documentation and initialize channelBuffer once.
@@ -36,21 +36,21 @@
*/
public class MgcpChannelInitializer extends ChannelInitializer<Channel> {
private static final int BUFFER_SIZE = 5000;
private static int channelBuffer;

This comment has been minimized.

@hrosa

hrosa Dec 18, 2017

Collaborator

this should not be static.

This comment has been minimized.

@ibrarahmad

ibrarahmad Dec 18, 2017

Contributor

Done

@@ -68,6 +68,8 @@ The default controller is based on MGCP protocol. Enabling MGCP is always requir
<controller protocol="mgcp">
<address>127.0.0.1</address>
<port>2427</port>
<port>2427</port>

This comment has been minimized.

@hrosa

hrosa Dec 18, 2017

Collaborator

Line duplicate. Please delete.

This comment has been minimized.

@ibrarahmad

ibrarahmad Dec 18, 2017

Contributor

Done

#476 Make MGCP channel buffer size configurable.
    Remove static from channelBuffer and fix a typo from documentation.
@hrosa

hrosa approved these changes Dec 19, 2017

@hrosa hrosa merged commit cdecdcc into RestComm:master Dec 19, 2017

@hrosa

This comment has been minimized.

Collaborator

hrosa commented Dec 19, 2017

Congrats @ibrarahmad on your first contribution to MS :)
Good job!

@ibrarahmad

This comment has been minimized.

Contributor

ibrarahmad commented Dec 26, 2017

@gsaslis

This comment has been minimized.

Contributor

gsaslis commented Dec 27, 2017

Nice job @ibrarahmad !!

Kudos on getting your first PR merged here (as you've seen by now, it's not as easy as it originally seems ;) ). Looking forward to many more!! ; )

P.S: Also added you to RestComm Contributors Hall of Fame. 🏆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment