Skip to content

Conversation

@acogoluegnes
Copy link
Contributor

Fixes #324

* @since 4.4.0
* @see ConnectionFactoryConfigurer
*/
public void load(String propertyFileLocation) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Configurator is a more common word in English.


/**
* Load settings from a property file.
* @param propertyFileLocation location of the property file to use
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not really clear from this line how the prefix is used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a "see ConnectionFactoryConfigurer" link would help the most here.

public static final String PORT = "port";
public static final String REQUESTED_CHANNEL_MAX = "requested.channel.max";
public static final String REQUESTED_FRAME_MAX = "requested.frame.max";
public static final String REQUESTED_HEARTBEAT = "requested.heartbeat";
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use "heartbeat.timeout" here. Same for frame.max and channel.max.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I tried to map with the existing properties in ConnectionFactory. Consistency over better naming? :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just weird that we use dots so aggressively. In case of requested.heartbeat we propagate an unfortunately named property and make it even more confusing by using dot separators.

Can we use connection.channel_max, connection.frame_max, connection.heartbeat.timeout for those 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.


public static final String USERNAME = "username";
public static final String PASSWORD = "password";
public static final String VIRTUAL_HOST = "virtual.host";
Copy link
Contributor

Choose a reason for hiding this comment

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

vhost?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Property name is virtualHost in ConnectionFactory, so I'm trying to be consistent. WDYT?

public static final String SHUTDOWN_TIMEOUT = "shutdown.timeout";
public static final String USE_DEFAULT_CLIENT_PROPERTIES = "use.default.client.properties";
public static final String CLIENT_PROPERTIES_PREFIX = "client.properties.";
public static final String AUTOMATIC_RECOVERY_ENABLED = "automatic.recovery.enabled";
Copy link
Contributor

Choose a reason for hiding this comment

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

This can probably be connection.recovery.enabled. We already have topology.recovery.enabled.

public static final String CLIENT_PROPERTIES_PREFIX = "client.properties.";
public static final String AUTOMATIC_RECOVERY_ENABLED = "automatic.recovery.enabled";
public static final String TOPOLOGY_RECOVERY_ENABLED = "topology.recovery.enabled";
public static final String NETWORK_RECOVERY_INTERVAL = "network.recovery.interval";
Copy link
Contributor

Choose a reason for hiding this comment

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

connection.recovery.interval?

@michaelklishin michaelklishin merged commit 34cce9b into 4.x.x-stable Nov 10, 2017
@gerhard gerhard deleted the rabbitmq-java-client-324 branch November 10, 2017 15:25
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