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
