You've cracked my elabourate webrev naming scheme. Thanks Daniel,
On 06/11/18 16:39, Daniel Fuchs wrote:
> Hi Rob,
>
> Assuming the new webrev is:
>
> http://cr.openjdk.java.net/~robm/8160768/webrev.09
>
> then it looks much better to me.
>
> I haven't re-reviewed everything in details -
Hi Rob,
Assuming the new webrev is:
http://cr.openjdk.java.net/~robm/8160768/webrev.09
then it looks much better to me.
I haven't re-reviewed everything in details - just looked
at the updates (LdapDnsProviderService,
LdapDnsProvider signature change, ServiceLocator)
BTW: if I'm not
Thanks folks,
Vyom, I've updated service to be volatile.
On 30/10/18 14:25, Daniel Fuchs wrote:
> Hi Rob,
>
> LdapCtxFactory.java
>
> 187 for (String u : r.getEndpoints()) {
> 188 try {
> 189 ctx = getLdapCtxFromUrl(
> 190
Hi Rob,
LdapCtxFactory.java
187 for (String u : r.getEndpoints()) {
188 try {
189 ctx = getLdapCtxFromUrl(
190 r.getDomainName(), new LdapURL(u), env);
191 } catch (NamingException e) {
192
Hi Rob,
Overall your code looks OK to me, In LdapDnsProviderService.java please
make 'service' as volatile otherwise 'getInstance()' may not work as
expected. I can see you implemented DCL but it may not work if 'service'
is not declare volatile.
If you like, you can us the initialization
GS IT PD LD PLM) ;
> sean.mul...@oracle.com
> Subject: [RFR] 8160768: Add capability to custom resolve host/domain names
> within the default JDNI LDAP provider
>
> This recently received CSR approval, so it seems like a good time to pick
> the codereview up again:
>
> h
Yes, thanks a lot Alan.
Vyom and Martin both had some very helpful feedback so I'm hoping to
hear from both!
-Rob
On 26/10/18 15:34, Alan Bateman wrote:
> On 25/10/2018 17:34, Rob McKenna wrote:
> > This recently received CSR approval, so it seems like a good time to pick
> > the codereview
On 25/10/2018 17:34, Rob McKenna wrote:
This recently received CSR approval, so it seems like a good time to pick
the codereview up again:
http://cr.openjdk.java.net/~robm/8160768/webrev.08/
I went through a number of iterations with Rob on the API/javadoc so I
think the API parts in
This recently received CSR approval, so it seems like a good time to pick
the codereview up again:
http://cr.openjdk.java.net/~robm/8160768/webrev.08/
Referencing:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050794.html
1) I'm copying the behaviour from
ri
> Cc: core-libs-dev@openjdk.java.net; Osipov, Michael (GS IT PD LD PLM)
> Subject: Re: [RFR] 8160768: Add capability to custom resolve host/domain
> names within the default JDNI LDAP provider
>
> Thanks Vyom, these should be fixed in:
>
> http://cr.openjdk.java.net/~robm/8160768/webrev
d capability to custom resolve host/domain names
within the default JDNI LDAP provider
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,
>
> Pleas
Hi Rob,
Latest code looks good to me.
minor bit.
LdapDnsProviderService.java
Line: 71 , while loop condition is bit complex, we can simplify it little bit
as follows.This will make code more readable and easy to understand.
while (iterator.hasNext())
{
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.
>
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
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
Hi folks, I'd like to revisit this for JDK10 as it was judged to be a
bit late in the game for 9.
I've published a new webrev at:
http://cr.openjdk.java.net/~robm/8160768/webrev.05/ which incorporates
very helpful feedback from Michael Osipov.
Notes:
- The 9 request rejection noted that there
Hi Rob,
On 02/02/17 22:14, Rob McKenna wrote:
Thanks Daniel,
New webrev at http://cr.openjdk.java.net/~robm/8160768/webrev.04/
In addition to your comments below I've added the package access check
we discussed.
Note: when a provider returns multiple results we create an LdapCtx
based on the
Hi Rob,
Most of the code changes looks fine except new code will throw NPE if
"ServiceLocator.mapDnToDomainName(ldapUrl.getDN())" is not able to map.
you have to check for null in LdapCtxFactory.java class as follows.
if(ServiceLocator.mapDnToDomainName(ldapUrl.getDN())!=null){
Thanks Daniel,
New webrev at http://cr.openjdk.java.net/~robm/8160768/webrev.04/
In addition to your comments below I've added the package access check
we discussed.
Note: when a provider returns multiple results we create an LdapCtx
based on the first of those. I think it might be useful to
Hi Rob,
This is not a code I know well - but here are a couple
of comments:
LdapCtxFactory.java:
168 NamingException ne = new NamingException();
232 NamingException ne = new NamingException();
Creating an exception is somewhat costly as it records the
backtrace in the
New webrev with updated test here:
http://cr.openjdk.java.net/~robm/8160768/webrev.03/
-Rob
On 26/01/17 04:03, Rob McKenna wrote:
> Ack, apologies, thats what I get for rushing. (which I suppose I'm doing
> again) That code was actually supposed to be in the getDnsUrls method.
> Updated in
Ack, apologies, thats what I get for rushing. (which I suppose I'm doing
again) That code was actually supposed to be in the getDnsUrls method.
Updated in place:
http://cr.openjdk.java.net/~robm/8160768/webrev.02/
I'll add a couple of tests for the SecurityManager along with some input
checking
Hi Rob,
I believe you should move the security check to before
the class is actually loaded, before the call to
171 List urls = getDnsUrls(url, env);
best regards,
-- daniel
On 25/01/17 17:44, Rob McKenna wrote:
I neglected to include a security check so I've cribbed the one
I neglected to include a security check so I've cribbed the one from
OBJECT_FACTORIES (NamingManager.setObjectFactoryBuilder()) - see:
http://cr.openjdk.java.net/~robm/8160768/webrev.02/
Note, this wraps the SecurityException in a NamingException. Presumably
its better to throw something than
Hi folks,
I'm looking for feedback on this suggested fix for the following bug:
https://bugs.openjdk.java.net/browse/JDK-816076
http://cr.openjdk.java.net/~robm/8160768/webrev.01/
This is something that has come up a few times. Basically in certain
environments (e.g. MS Active Directory) there
25 matches
Mail list logo
