[RFR] 8160768: Add capability to custom resolve host/domain names within the default JDNI LDAP provider

[vyom tewari]

Hi Rob,

Please find below comments.

DefaultLdapDnsProvider.java

  line 35:     convert it to for-each code will be more readable.

LdapDnsProviderService.java

  line 21: constant name does not follow Java naming convention, there 
are other places as well please fix it.

getInstance() is already synchronized why you need another 
"synchronized" block ?

Line 70: Please use result.getEndpoints().isEmpty() in place of 
result.getEndpoints().size() == 0

LdapDnsProvider.java

constructor LdapDnsProvider() calls LdapDnsProvider(Void unused) which 
does nothing, can you explain why LdapDnsProvider()  calls 
LdapDnsProvider(Void unused) ?

LdapDnsProviderResult.java

   Private field  domainName & endpoints both can be final.

LdapDnsProviderTest.java

Fix the tag Order it is not correct. correct order is as follows.

/**
  * @test
  * @bug 8160768
  * @summary ctx provider tests for ldap
  * @modules java.naming/com.sun.jndi.ldap
  * @compile dnsprovider/TestDnsProvider.java
  * @run main/othervm LdapDnsProviderTest
  * @run main/othervm LdapDnsProviderTest nosm
  * @run main/othervm LdapDnsProviderTest smnodns
  * @run main/othervm LdapDnsProviderTest smdns
  */

Line 82,83 you don't need System.out.println(e); e.printStackTrace();

Line 70: you don't need this try block

Line 96 : constant name does not follow Java naming convention

Line 105: you can use try-with-resources this will avoid some boilerplate.

Thanks,
Vyom

On Tuesday 05 December 2017 11:18 PM, Rob McKenna wrote:
> As this enhancement is limited to JDK10 this frees us up to explore a
> different approach.
>
> http://cr.openjdk.java.net/~robm/8160768/webrev.06/
>
> In this webrev we're using the Service Provider Interface to load an
> implementation of the new LdapDnsProvider class. Implementations of this
> class are responsible for resolving DNS endpoints for a given ldap url
> should the default implementation be insufficient in a particular
> environment.
>
> Note: if a security manager is installed the ldapDnsProvider
> RuntimePermission must be granted.
>
>      -Rob
>