-
Notifications
You must be signed in to change notification settings - Fork 583
Add property file-based initialization #329
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
Conversation
* @see ConnectionFactoryConfigurer | ||
*/ | ||
public void load(String propertyFileLocation) throws IOException { | ||
ConnectionFactoryConfigurer.load(this, propertyFileLocation); |
There was a problem hiding this comment.
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 | ||
* @param prefix prefix used in the property file keys |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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? :-)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vhost
?
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connection.recovery.interval
?
Fixes #324