> On Aug. 6, 2013, 11:40 p.m., Sheng Yang wrote: > > api/src/com/cloud/network/Networks.java, line 135 > > <https://reviews.apache.org/r/12849/diff/5/?file=337785#file337785line135> > > > > I think by following the previous case, it should be > > URI(scheme,value.toString()) rather than "null, null". It possibly doesn't > > make difference, but it's better not to use different constructor. > > > > The same with several URI constructor above. > > > > And I doubt if it's really need to discard this check. At least they > > would working for other broadcast domain type. > > Sheng Yang wrote: > Dave explained this. Please drop this issue.
Thanks for the quick reply Sheng! Actually, I dug deeper and realized that if we use the 4-parameter URI constructor, the "value" parameter is interpreted as host, and colons wouldn't be allowed! It looks like the only way to preserve the behavior as it was is to stick with the old string concatenation behavior of scheme + "://" + value. In fact, this is what Daan and I agreed on in IRC yesterday, but it looks like he went with the 4 param constructor - I'll discuss with him later to see what changed. - Dave ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12849/#review24756 ----------------------------------------------------------- On Aug. 6, 2013, 2:25 p.m., daan Hoogland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12849/ > ----------------------------------------------------------- > > (Updated Aug. 6, 2013, 2:25 p.m.) > > > Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik Das, > and Sheng Yang. > > > Repository: cloudstack-git > > > Description > ------- > > Both BroadcastDomainType and IsolationType needed some extra code for > backwards compatibility > > > Diffs > ----- > > api/src/com/cloud/network/Networks.java c76c3d4 > api/test/com/cloud/network/NetworksTest.java 31114e8 > > Diff: https://reviews.apache.org/r/12849/diff/ > > > Testing > ------- > > > Thanks, > > daan Hoogland > >
