Thanks Vyom, these should be fixed in: http://cr.openjdk.java.net/~robm/8160768/webrev.07/
Comments inline: On 06/12/17 22:14, vyom tewari wrote: > 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 ? Ah, neglected to remove the outer synchronized keyword, thanks. > > 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) ? I've copied this pattern from the System.LoggerFinder api and I had forgotten what it was for too: http://www.oracle.com/technetwork/java/seccodeguide-139067.html#7 Section 7.3. > > 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(); I'm going to leave these in as I've had a few failing tests in the past where I've needed to push diagnostic changes like this to determine the root cause. -Rob > > 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 > > >