Skip to content
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

removing deprecated client config methods #15012

Merged

Conversation

@sancar
Copy link
Member

sancar commented May 10, 2019

No description provided.

@sancar sancar added this to the 4.0 milestone May 10, 2019
@sancar sancar self-assigned this May 10, 2019
@sancar

This comment has been minimized.

Copy link
Member Author

sancar commented May 21, 2019

I have noticed that ClientConfigXmlGenerator does not support discovery strategies except AWS. I have opened a separate issue for that.
#15010

@sancar

This comment has been minimized.

Copy link
Member Author

sancar commented May 21, 2019

run-lab-run

@sancar sancar force-pushed the sancar:cleanup/depreacatedClientConfigs/master branch from 1ac3c1f to 9d73d40 May 21, 2019
@sancar sancar force-pushed the sancar:cleanup/depreacatedClientConfigs/master branch from 9d73d40 to 0696e4b Jun 11, 2019
@sancar sancar marked this pull request as ready for review Jun 12, 2019
@sancar sancar requested review from leszko, ihsandemir and asimarslan Jun 12, 2019
@sancar sancar force-pushed the sancar:cleanup/depreacatedClientConfigs/master branch from 0696e4b to eda75f8 Jun 12, 2019
@leszko
leszko approved these changes Jun 12, 2019
Copy link
Contributor

leszko left a comment

@sancar Good change 👍 I've added some minor comments. PTAL.

@@ -497,13 +498,13 @@ private static void ssl(XmlGenerator gen, SSLConfig ssl) {
.close();
}

private static void aws(XmlGenerator gen, ClientAwsConfig aws) {
private static void aws(XmlGenerator gen, AwsConfig aws) {

This comment has been minimized.

Copy link
@leszko

leszko Jun 12, 2019

Contributor

I think that we may not need this method at all now. We don't have it for others ("gcp", "kubernetes"), so I guess it was left only because of the deprecated ClientAwsConfig.

This comment has been minimized.

Copy link
@sancar

sancar Jun 13, 2019

Author Member

#15012 (comment)
Did you see my comment here? When I add a test to ClientConfigXmlGeneratorTest as follows I can see that it fails:

@Test
    public void testKubernetes() {
        KubernetesConfig kubernetesConfig = clientConfig.getNetworkConfig().getKubernetesConfig();
        kubernetesConfig.setEnabled(true);

        KubernetesConfig kubernetesConfig1 = newConfigViaGenerator().getNetworkConfig().getKubernetesConfig();
        assertTrue(kubernetesConfig1.isEnabled());
    }

So, I think aws support is there but others are missing. They needs to be added. Hence I open the issue. Does that make sense ? @leszko

This comment has been minimized.

Copy link
@leszko

leszko Jun 13, 2019

Contributor

@sancar thanks for clarifying.

  1. You're right that the support for other strategies (kubernetes, gcp, etc.) is missing. Please open an issue.

  2. However, even though AWS support is there, you can remove it, because it's deprecated. It's parameters are not up to date (you can compare to the current AWS Discovery Strategy Parameters). We should remove it and add the generic solution like it's done here.

@@ -194,8 +195,8 @@ public void networkSsl() {

@Test
public void networkAws() {

This comment has been minimized.

Copy link
@leszko

leszko Jun 12, 2019

Contributor

The same here, I think we don't need this method, since the behavior goes now into the generic case (all aliased discovery strategies: "aws", "gcp", "kubernetes")

@@ -87,7 +87,7 @@ public HazelcastInstance newHazelcastClient(ClientConfig config) {
private AddressProvider createAddressProvider(ClientConfig config) {
boolean discoveryEnabled = new HazelcastProperties(config.getProperties())
.getBoolean(ClientProperty.DISCOVERY_SPI_ENABLED);
ClientAwsConfig awsConfig = config.getNetworkConfig().getAwsConfig();
AwsConfig awsConfig = config.getNetworkConfig().getAwsConfig();

This comment has been minimized.

Copy link
@leszko

leszko Jun 12, 2019

Contributor

This is also suspicious, I guess we shouldn't need it now.

@leszko
leszko approved these changes Jun 14, 2019
Copy link
Contributor

leszko left a comment

👍

@sancar sancar merged commit 25516e4 into hazelcast:master Jul 3, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@sancar sancar deleted the sancar:cleanup/depreacatedClientConfigs/master branch Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.