Generalize PulsarConfigurationLoader #168
Conversation
|
CLA is valid! |
2 similar comments
|
CLA is valid! |
|
CLA is valid! |
rdhabalia
added
the
type/enhancement
merlimat
left a comment
merlimat
left a comment
There was a problem hiding this comment.
Looks good, just couple of minor comments
There was a problem hiding this comment.
Are we using the properties anywhere other than when loading the config file ?
There was a problem hiding this comment.
No. So, made PulsarConfiguration interface and implementation config class will implement it.
There was a problem hiding this comment.
Should have a create that doesn't take any properties? In general we should try to avoid using System.getProperties() since it can lead to unpredictable tests, depending on what was passed on the command line.
There was a problem hiding this comment.
that's true. Made create(Properties) private and made appropriate test-case changes.
maskit
left a comment
maskit
left a comment
There was a problem hiding this comment.
This change looks familiar. It might be a change I sent before?
It looks some of tests doesn't make sense. Should check if the modified test cases are valid.
There was a problem hiding this comment.
This test case doesn't test anything about PulsarConfigurationLoader. It should call PulsarConfigurationLoader.create(prop, ServiceConfiguration.class).
There was a problem hiding this comment.
This one too. It should use PulsarConfigurationLoader.
There was a problem hiding this comment.
I think this class should be renamed to PulsarConfigurationLoaderTest, because ServiceConfigurationLoader is renamed to PulsarConfigurationLoader.
Also, this class was a test class for ServiceConfigurationLoader but not for ServiceConfiguration. I guess this naming mismatch causes a misread, which is the reason why some of test methods are (incorrectly) simply creating a ServiceConfiguration instance without PulsarConfigurationLoader.
rdhabalia
force-pushed
the
config
branch
2 times, most recently
from
10f221a to
d9b36c6
Compare
|
@maskit .. Yes, after introducing separate configs for broker, discovery and websocket service, it makes sense to get the config-generalization change which we missed earlier to bring it here. Also fixed the test-cases for PulsarConfigurationLoader. |
maskit
left a comment
maskit
left a comment
There was a problem hiding this comment.
Thanks for the fix. Looks good to me.
* Improve pulsar functions logging - inject instance id in thread context. so we can have instance id information in function log - rename dlog logging to bk logging, so all bk/dlog logging can be routed there - add more variables to allow overrides by sys variables - fixes some scripts * Improve the logic for finding java instance location * Improve configuring log directory for pulsar worker * Fix python location * Fix license header
### Motivation Add seek by messageID
…taIfMissing… (apache#168) * Added a new MetadataUtils with an implementation of createKafkaMetadataIfMissing that coes not overwrite the cluster metadata. Modified several use cases to support the change. Logic for the createKafkaMetadataIfMissing implementation is as follows: - If the tenant does not exist it will be created - If the tenant exists but the allowedClusters list does not include the cluster this method will add the cluster to the allowedClusters list - If the namespace does not exist it will be created - If the namespace exists but the replicationClusters list does not include the cluster this method will add the cluster to the replicationClusters list - If the offset topic does not exist it will be created - If the offset topic exists but some partitions are missing, the missing partitions will be created * Fixing checkstyle errors Co-authored-by: William Mccarley <mccarwi@win.eng.vzwnet.com>
…ache#445) This case is usually encountered when attempting to use KoP with standalone mode. When new behavior was introduced in apache#168 standalone mode was broken due to sample namespace setup etc not taking place until after the broker is initialized (which in turn initializes protocol handlers). Fixes apache#185
…ry (apache#168) (cherry picked from commit 335ab8a) Conflicts: tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedReadHandleImpl.java
4 participants
Motivation
Generalizing PulsarConfigurationLoader helps to load different
ServiceConfigurationofBrokerService,DiscoveryService,WebSocketProxyin a generic way without adding additional logic-layer to parse those config.Modifications
Generalizing "Config" Parsing for all the services such as
BrokerService,DiscoveryService,WebSocketProxyResult
No functionality change.