AWS CloudMap as a discovery mode for the peer forwarder #734
Conversation
…arder plugin. Addionally, refactored the interactions within the PeerListProviderFactory and DiscoveryMode classes. opendistro-for-elasticsearch#730 Signed-off-by: David Venable <dlv@amazon.com>
…and close these resources during testing. Use Awaitility for improved timing and to aid in reliable testing. opendistro-for-elasticsearch#730 Signed-off-by: David Venable <dlv@amazon.com>
.../amazon/dataprepper/plugins/prepper/peerforwarder/discovery/AwsCloudMapPeerListProvider.java
Outdated
Show resolved
Hide resolved
.../amazon/dataprepper/plugins/prepper/peerforwarder/discovery/AwsCloudMapPeerListProvider.java
Show resolved
Hide resolved
.../amazon/dataprepper/plugins/prepper/peerforwarder/discovery/AwsCloudMapPeerListProvider.java
Outdated
Show resolved
Hide resolved
.../amazon/dataprepper/plugins/prepper/peerforwarder/discovery/AwsCloudMapPeerListProvider.java
Outdated
Show resolved
Hide resolved
.../amazon/dataprepper/plugins/prepper/peerforwarder/discovery/AwsCloudMapPeerListProvider.java
Outdated
Show resolved
Hide resolved
.../amazon/dataprepper/plugins/prepper/peerforwarder/discovery/AwsCloudMapPeerListProvider.java
Outdated
Show resolved
Hide resolved
.../amazon/dataprepper/plugins/prepper/peerforwarder/discovery/AwsCloudMapPeerListProvider.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| protected void doCloseAsync(CompletableFuture<?> future) { |
There was a problem hiding this comment.
Will this be invoked when the server is stopped or on SIGKILL signal ?
There was a problem hiding this comment.
This is large part depends on how the Peer Forwarder uses the list. This is a Closeable interface, much like the DnsAddressEndpointGroup class. So the caller will need to close it in order for this to run.
.../com/amazon/dataprepper/plugins/prepper/peerforwarder/discovery/PeerListProviderFactory.java
Show resolved
Hide resolved
…s, adding final modifiers, and formatting in the main files modified by this PR. opendistro-for-elasticsearch#730 Signed-off-by: David Venable <dlv@amazon.com>
There was a problem hiding this comment.
One of the test cases is failing:
com.amazon.dataprepper.plugins.prepper.peerforwarder.discovery.AwsCloudMapPeerListProviderTest > WithDiscoverInstances > getPeerList_returns_list_as_found() FAILED
org.awaitility.core.ConditionTimeoutException at AwsCloudMapPeerListProviderTest.java:204
Caused by: java.lang.AssertionError at MatcherAssert.java:20
…pPeerListProviderTest. opendistro-for-elasticsearch#730 Signed-off-by: David Venable <dlv@amazon.com>
.../amazon/dataprepper/plugins/prepper/peerforwarder/discovery/AwsCloudMapPeerListProvider.java
Show resolved
Hide resolved
|
|
||
| * `awsCloudMapNamespaceName` - Set to your CloudMap Namespace name | ||
| * `awsCloudMapServiceName` - Set to the service name within your specified Namespace | ||
| * `awsRegion` - The AWS region where your namespace exists. |
There was a problem hiding this comment.
STY: Can we make the above snake case? Just for style consistency.
There was a problem hiding this comment.
Looks like we have the same issue in otel source. Maybe we can change them in separate PR
There was a problem hiding this comment.
This is an inconsistency which I am seeing throughout the project. Since the awsRegion already existed, and I'm using it in my changes, I decided to use camelCase for my new properties.
| pluginMetrics); | ||
| } | ||
|
|
||
| private static String getRequiredSettingString(final PluginSetting pluginSetting, final String propertyName) { |
There was a problem hiding this comment.
We might consider refactoring it into data-prepper API if it is used in multiple places. Just a note for now
There was a problem hiding this comment.
Yes. I think this also relates to another issue that came up. We have different exceptions for plugin errors. We should make these consistent.
.../main/java/com/amazon/dataprepper/plugins/prepper/peerforwarder/discovery/DiscoveryMode.java
Show resolved
Hide resolved
...zon/dataprepper/plugins/prepper/peerforwarder/discovery/AwsCloudMapPeerListProviderTest.java
Outdated
Show resolved
Hide resolved
…opendistro-for-elasticsearch#730 Signed-off-by: David Venable <dlv@amazon.com>
Issue #, if available:
#730
Description of changes:
This PR includes:
AwsCloudMapPeerListProviderclass provides this implementation.PeerListProviderFactoryandDiscoveryModeinteract. This change adds compile-time checks on adding newDiscoveryModevalues since you cannot add them without now including thecreatemethod.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.