You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by daan Hoogland <da...@gmail.com> on 2013/07/23 12:02:15 UTC
Review Request 12849: added backwards compatibility code to Networks enums
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/
-----------------------------------------------------------
Review request for cloudstack and Koushik Das.
Repository: cloudstack-git
Description
-------
Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
Diffs
-----
api/src/com/cloud/network/Networks.java c76c3d4
server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java 6fafa3e
Diff: https://reviews.apache.org/r/12849/diff/
Testing
-------
Thanks,
daan Hoogland
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
> On July 23, 2013, 12:30 p.m., Koushik Das wrote:
> > api/src/com/cloud/network/Networks.java, line 225
> > <https://reviews.apache.org/r/12849/diff/1/?file=325398#file325398line225>
> >
> > Why only Vlan in case of IsolationType and Vlan, Storage and Mido in the case of BroadcastDomainType? Earlier both used similar logic.
> >
> > If all system VMs (SSVM, CPVM and router VMs) are getting started after this then should be fine.
> >
> > Also ensure that upgraded setups work fine as well.
tested by instantiating a zone. There are no upgrade (database) aspects to this as far as I can see.
- daan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review23686
-----------------------------------------------------------
On July 23, 2013, 10:02 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated July 23, 2013, 10:02 a.m.)
>
>
> Review request for cloudstack and Koushik Das.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
>
>
> Diffs
> -----
>
> api/src/com/cloud/network/Networks.java c76c3d4
> server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java 6fafa3e
>
> Diff: https://reviews.apache.org/r/12849/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> daan Hoogland
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Koushik Das <ko...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review23686
-----------------------------------------------------------
api/src/com/cloud/network/Networks.java
<https://reviews.apache.org/r/12849/#comment47581>
Why only Vlan in case of IsolationType and Vlan, Storage and Mido in the case of BroadcastDomainType? Earlier both used similar logic.
If all system VMs (SSVM, CPVM and router VMs) are getting started after this then should be fine.
Also ensure that upgraded setups work fine as well.
- Koushik Das
On July 23, 2013, 10:02 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated July 23, 2013, 10:02 a.m.)
>
>
> Review request for cloudstack and Koushik Das.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
>
>
> Diffs
> -----
>
> api/src/com/cloud/network/Networks.java c76c3d4
> server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java 6fafa3e
>
> Diff: https://reviews.apache.org/r/12849/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> daan Hoogland
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Toshiaki Hatano <to...@verio.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review24161
-----------------------------------------------------------
I tested this patch locally, VLAN isolated advanced zone with 1 KVM hypervisor.
Confirmed this patch resolve bug https://issues.apache.org/jira/browse/CLOUDSTACK-3682
Thanks!
- Toshiaki Hatano
On July 23, 2013, 10:02 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated July 23, 2013, 10:02 a.m.)
>
>
> Review request for cloudstack and Koushik Das.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
>
>
> Diffs
> -----
>
> api/src/com/cloud/network/Networks.java c76c3d4
> server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java 6fafa3e
>
> Diff: https://reviews.apache.org/r/12849/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> daan Hoogland
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by "Jenkins Cloudstack.org" <hu...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review23683
-----------------------------------------------------------
Review 12849 PASSED the build test
The url of build cloudstack-master-with-patch #20 is : http://jenkins.cloudstack.org/job/cloudstack-master-with-patch/20/
- Jenkins Cloudstack.org
On July 23, 2013, 10:02 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated July 23, 2013, 10:02 a.m.)
>
>
> Review request for cloudstack and Koushik Das.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
>
>
> Diffs
> -----
>
> api/src/com/cloud/network/Networks.java c76c3d4
> server/src/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java 6fafa3e
>
> Diff: https://reviews.apache.org/r/12849/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> daan Hoogland
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Sheng Yang <sh...@yasker.org>.
> On Aug. 7, 2013, 1:10 a.m., Dave Cahill wrote:
> > api/src/com/cloud/network/Networks.java, line 135
> > <https://reviews.apache.org/r/12849/diff/5/?file=337785#file337785line135>
> >
> > Sheng, not sure if you caught the conversation around breaking backwards compatibility, but it explains these changes.
> >
> > the reason for using the 4-parameter URI constructor is that it creates "scheme://value" URIs, which is what this method used to do before it was changed on July 19. There is no 2-parameter URI constructor, so I guess you're proposing the 3-parameter constructor - however that creates a URI without // - see https://gist.github.com/davecahill/6170364 for an example.
> >
> > As for the check, this was also added in the July 19 commit - it causes Exceptions on valid input (colons in the value param are valid), which is why I'm proposing we remove it.
> >
> > Hope that explains it - feel free to grab me on IRC for more details if needed.
Got it, thanks for explaining!
- Sheng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review24766
-----------------------------------------------------------
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
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Dave Cahill <dc...@midokura.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review24766
-----------------------------------------------------------
Replied to Sheng's comments.
api/src/com/cloud/network/Networks.java
<https://reviews.apache.org/r/12849/#comment48888>
Sheng, not sure if you caught the conversation around breaking backwards compatibility, but it explains these changes.
the reason for using the 4-parameter URI constructor is that it creates "scheme://value" URIs, which is what this method used to do before it was changed on July 19. There is no 2-parameter URI constructor, so I guess you're proposing the 3-parameter constructor - however that creates a URI without // - see https://gist.github.com/davecahill/6170364 for an example.
As for the check, this was also added in the July 19 commit - it causes Exceptions on valid input (colons in the value param are valid), which is why I'm proposing we remove it.
Hope that explains it - feel free to grab me on IRC for more details if needed.
- Dave Cahill
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
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Dave Cahill <dc...@midokura.com>.
> 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
>
>
Re: Review Request 12849: added backwards compatibility code to
Networks enums
Posted by Daan Hoogland <da...@gmail.com>.
Donal,
The mix of vlan://<number> and <number> is one of the issues i had to deal
with doing this. The vlan not being handled by my first patch correctly is
why this second one was made. for vlans it reverts what I did before. What
is it you are seeing? And how does it relate to the BroadcastDomainType?
The code you refer to expects something like '<scheme>://<number>'. The
reason mido needs the string concatination is because it can contain
'mido://<somestring>:<someotherstring>'.
Daan
On Wed, Aug 7, 2013 at 7:12 PM, Donal Lafferty <do...@citrix.com>wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
>
> On August 6th, 2013, 11:40 p.m. UTC, *Sheng Yang* wrote:
>
> api/src/com/cloud/network/Networks.java<https://reviews.apache.org/r/12849/diff/5/?file=337785#file337785line79> (Diff
> revision 5)
>
> public class Networks {
>
> 79
>
> return new URI("vlan", value.toString(), null, null);
>
> I don't quite understand this part. Why "//" is missing in this case? Who would require "//" and who wouldn't?
>
> On August 7th, 2013, 8:42 a.m. UTC, *daan Hoogland* wrote:
>
> Only the mido code needs the explicit // the rest can deal with URI constructors
>
> Are you sure? I'm seeing problems with CitrixResourceBase.getNetwork(CitrixResourceBase.java:1043):
>
> long vlan = Long.parseLong(broadcastUri.getHost());
>
>
>
> - Donal
>
> On August 7th, 2013, 9:40 a.m. UTC, daan Hoogland wrote:
> Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik
> Das, and Sheng Yang.
> By daan Hoogland.
>
> *Updated Aug. 7, 2013, 9:40 a.m.*
> *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)
>
> View Diff <https://reviews.apache.org/r/12849/diff/>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Donal Lafferty <do...@citrix.com>.
> On Aug. 6, 2013, 11:40 p.m., Sheng Yang wrote:
> > api/src/com/cloud/network/Networks.java, line 79
> > <https://reviews.apache.org/r/12849/diff/5/?file=337785#file337785line79>
> >
> > I don't quite understand this part. Why "//" is missing in this case? Who would require "//" and who wouldn't?
>
> daan Hoogland wrote:
> Only the mido code needs the explicit // the rest can deal with URI constructors
Are you sure? I'm seeing problems with CitrixResourceBase.getNetwork(CitrixResourceBase.java:1043):
long vlan = Long.parseLong(broadcastUri.getHost());
- Donal
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review24756
-----------------------------------------------------------
On Aug. 7, 2013, 9:40 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2013, 9:40 a.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
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
> On Aug. 6, 2013, 11:40 p.m., Sheng Yang wrote:
> > api/src/com/cloud/network/Networks.java, line 147
> > <https://reviews.apache.org/r/12849/diff/5/?file=337785#file337785line147>
> >
> > No reference of getTypeOf(URI) in the code?
this code supplies facilities for features that are being ported to the master branch
> On Aug. 6, 2013, 11:40 p.m., Sheng Yang wrote:
> > api/src/com/cloud/network/Networks.java, line 159
> > <https://reviews.apache.org/r/12849/diff/5/?file=337785#file337785line159>
> >
> > Does this function need to be public? I think getSchemeValue() and getTypeOf(String str) should be enough?
see the comment above
- daan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review24756
-----------------------------------------------------------
On Aug. 7, 2013, 8:50 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2013, 8:50 a.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
>
>
Re: Review Request 12849: added backwards compatibility code to
Networks enums
Posted by Daan Hoogland <da...@gmail.com>.
Sheng,
Some of this code will be used in a feature allowing vpc gateways to be
plugged to sdn networks and to allow multiple gateways to share that
network. I am using that in a hack on 4.1.1 and porting the code to master
now, bit by bit so to speak.
This is why you see some functions that seem not to be used.
regards,
Daan
On Wed, Aug 7, 2013 at 8:52 AM, Daan Hoogland <da...@gmail.com>wrote:
> Hi Dave, I see what you mean by colons wouldn't be allowed. Nothing
> changed but I went for this solution for consistency (over backward
> compatibility). I will change again. Let me get to work first. nice working
> around the globe;)
>
> Daan
>
>
> On Wed, Aug 7, 2013 at 3:57 AM, Sheng Yang <sh...@yasker.org> wrote:
>
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/12849/
>>
>> On August 6th, 2013, 11:40 p.m. UTC, *Sheng Yang* wrote:
>>
>> api/src/com/cloud/network/Networks.java<https://reviews.apache.org/r/12849/diff/5/?file=337785#file337785line135> (Diff
>> revision 5)
>>
>> public String scheme() {
>>
>> 95
>>
>> // do we need to check that value does not contain a scheme
>>
>> 135
>>
>> return new URI(scheme,value.toString(),null,null);
>>
>> 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.
>>
>> On August 7th, 2013, 1:45 a.m. UTC, *Sheng Yang* wrote:
>>
>> Dave explained this. Please drop this issue.
>>
>> On August 7th, 2013, 1:54 a.m. UTC, *Dave Cahill* wrote:
>>
>> 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.
>>
>> Thank you Dave!
>>
>>
>> - Sheng
>>
>> On August 6th, 2013, 2:25 p.m. UTC, daan Hoogland wrote:
>> Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik
>> Das, and Sheng Yang.
>> By daan Hoogland.
>>
>> *Updated Aug. 6, 2013, 2:25 p.m.*
>> *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)
>>
>> View Diff <https://reviews.apache.org/r/12849/diff/>
>>
>
>
Re: Review Request 12849: added backwards compatibility code to
Networks enums
Posted by Daan Hoogland <da...@gmail.com>.
Hi Dave, I see what you mean by colons wouldn't be allowed. Nothing
changed but I went for this solution for consistency (over backward
compatibility). I will change again. Let me get to work first. nice working
around the globe;)
Daan
On Wed, Aug 7, 2013 at 3:57 AM, Sheng Yang <sh...@yasker.org> wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
>
> On August 6th, 2013, 11:40 p.m. UTC, *Sheng Yang* wrote:
>
> api/src/com/cloud/network/Networks.java<https://reviews.apache.org/r/12849/diff/5/?file=337785#file337785line135> (Diff
> revision 5)
>
> public String scheme() {
>
> 95
>
> // do we need to check that value does not contain a scheme
>
> 135
>
> return new URI(scheme,value.toString(),null,null);
>
> 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.
>
> On August 7th, 2013, 1:45 a.m. UTC, *Sheng Yang* wrote:
>
> Dave explained this. Please drop this issue.
>
> On August 7th, 2013, 1:54 a.m. UTC, *Dave Cahill* wrote:
>
> 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.
>
> Thank you Dave!
>
>
> - Sheng
>
> On August 6th, 2013, 2:25 p.m. UTC, daan Hoogland wrote:
> Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik
> Das, and Sheng Yang.
> By daan Hoogland.
>
> *Updated Aug. 6, 2013, 2:25 p.m.*
> *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)
>
> View Diff <https://reviews.apache.org/r/12849/diff/>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Sheng Yang <sh...@yasker.org>.
> 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.
>
> Dave Cahill wrote:
> 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.
Thank you Dave!
- Sheng
-----------------------------------------------------------
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
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Sheng Yang <sh...@yasker.org>.
> 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.
Dave explained this. Please drop this issue.
- Sheng
-----------------------------------------------------------
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
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
> On Aug. 6, 2013, 11:40 p.m., Sheng Yang wrote:
> > api/src/com/cloud/network/Networks.java, line 79
> > <https://reviews.apache.org/r/12849/diff/5/?file=337785#file337785line79>
> >
> > I don't quite understand this part. Why "//" is missing in this case? Who would require "//" and who wouldn't?
Only the mido code needs the explicit // the rest can deal with URI constructors
- daan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review24756
-----------------------------------------------------------
On Aug. 7, 2013, 8:38 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2013, 8:38 a.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
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Sheng Yang <sh...@yasker.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review24756
-----------------------------------------------------------
api/src/com/cloud/network/Networks.java
<https://reviews.apache.org/r/12849/#comment48880>
I don't quite understand this part. Why "//" is missing in this case? Who would require "//" and who wouldn't?
api/src/com/cloud/network/Networks.java
<https://reviews.apache.org/r/12849/#comment48877>
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.
api/src/com/cloud/network/Networks.java
<https://reviews.apache.org/r/12849/#comment48879>
No reference of getTypeOf(URI) in the code?
api/src/com/cloud/network/Networks.java
<https://reviews.apache.org/r/12849/#comment48878>
Does this function need to be public? I think getSchemeValue() and getTypeOf(String str) should be enough?
- Sheng Yang
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
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Dave Cahill <dc...@midokura.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review24787
-----------------------------------------------------------
Three minor comments, but my main concerns are addressed by this diff.
api/src/com/cloud/network/Networks.java
<https://reviews.apache.org/r/12849/#comment48907>
Should this say isolation URI instead of broadcast URI?
api/test/com/cloud/network/NetworksTest.java
<https://reviews.apache.org/r/12849/#comment48905>
Not important, but I think this should be "of broadcasttype lswitch".
api/test/com/cloud/network/NetworksTest.java
<https://reviews.apache.org/r/12849/#comment48906>
Not important, but I think this should be "of broadcasttype mido".
- Dave Cahill
On Aug. 7, 2013, 8:50 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2013, 8:50 a.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
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Hugo Trippaers <ht...@schubergphilis.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review25215
-----------------------------------------------------------
Ship it!
With no open issues and all review items processed i'm shipping this patch.
- Hugo Trippaers
On Aug. 15, 2013, 7:53 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated Aug. 15, 2013, 7:53 a.m.)
>
>
> Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik Das, Hugo Trippaers, and Sheng Yang.
>
>
> Bugs: CLOUDSTACK-4345
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility.
>
> All over the code calls are done to URI.getHost() to retrieve ids of broadcastdomains. These id obviously are not hosts so this call is confusing and requires maintenance all over the code base. Also for different types the value returned by getHost has different meaning. vlan://1 is id 1 of course but others might be ranges of vlans or colon separated values. To make things worse a NiciraNvp has an uri of the form lswitch:<uuid> without the forward slashes.
>
> To make the system more maintainable in this perspect the changes in this patch were made. It is my intention to replace the calls to getHost by the member call getValueFrom or the static method getValue in time. In this way maintenance is centralized and an overview of differnces and quirks is easily found
>
>
> 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
> -------
>
> unit tests for different kind of BroadcastDomainType.values
>
>
> Thanks,
>
> daan Hoogland
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/
-----------------------------------------------------------
(Updated Aug. 15, 2013, 7:53 a.m.)
Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik Das, Hugo Trippaers, and Sheng Yang.
Changes
-------
linked to a subtask that this relates to
Bugs: CLOUDSTACK-4345
Repository: cloudstack-git
Description
-------
Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility.
All over the code calls are done to URI.getHost() to retrieve ids of broadcastdomains. These id obviously are not hosts so this call is confusing and requires maintenance all over the code base. Also for different types the value returned by getHost has different meaning. vlan://1 is id 1 of course but others might be ranges of vlans or colon separated values. To make things worse a NiciraNvp has an uri of the form lswitch:<uuid> without the forward slashes.
To make the system more maintainable in this perspect the changes in this patch were made. It is my intention to replace the calls to getHost by the member call getValueFrom or the static method getValue in time. In this way maintenance is centralized and an overview of differnces and quirks is easily found
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
-------
unit tests for different kind of BroadcastDomainType.values
Thanks,
daan Hoogland
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/
-----------------------------------------------------------
(Updated Aug. 13, 2013, 1:58 p.m.)
Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik Das, Hugo Trippaers, and Sheng Yang.
Changes
-------
added javadoc
Repository: cloudstack-git
Description
-------
Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility.
All over the code calls are done to URI.getHost() to retrieve ids of broadcastdomains. These id obviously are not hosts so this call is confusing and requires maintenance all over the code base. Also for different types the value returned by getHost has different meaning. vlan://1 is id 1 of course but others might be ranges of vlans or colon separated values. To make things worse a NiciraNvp has an uri of the form lswitch:<uuid> without the forward slashes.
To make the system more maintainable in this perspect the changes in this patch were made. It is my intention to replace the calls to getHost by the member call getValueFrom or the static method getValue in time. In this way maintenance is centralized and an overview of differnces and quirks is easily found
Diffs (updated)
-----
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
-------
unit tests for different kind of BroadcastDomainType.values
Thanks,
daan Hoogland
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Toshiaki Hatano <to...@verio.net>.
> On Aug. 9, 2013, 5:23 p.m., Toshiaki Hatano wrote:
> > api/src/com/cloud/network/Networks.java, line 172
> > <https://reviews.apache.org/r/12849/diff/9/?file=338752#file338752line172>
> >
> > Why don't we merge getValueFrom() and getValue()?
> >
> > public String getValue(URI uri) {
> > if(uri.isOpaque()) {
> > return uri.getSchemeSpecificPart();
> > } else {
> > return uri.getHost();
> > }
> > }
Or, maybe make getValueFrom private.
I think it will make confuse if we provide 2 public method to get value of URI.
- Toshiaki
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review24925
-----------------------------------------------------------
On Aug. 8, 2013, 11:51 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2013, 11:51 a.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.
>
> All over the code calls are done to URI.getHost() to retrieve ids of broadcastdomains. These id obviously are not hosts so this call is confusing and requires maintenance all over the code base. Also for different types the value returned by getHost has different meaning. vlan://1 is id 1 of course but others might be ranges of vlans or colon separated values. To make things worse a NiciraNvp has an uri of the form lswitch:<uuid> without the forward slashes.
>
> To make the system more maintainable in this perspect the changes in this patch were made. It is my intention to replace the calls to getHost by the member call getValueFrom or the static method getValue in time. In this way maintenance is centralized and an overview of differnces and quirks is easily found
>
>
> 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
> -------
>
> unit tests for different kind of BroadcastDomainType.values
>
>
> Thanks,
>
> daan Hoogland
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Toshiaki Hatano <to...@verio.net>.
> On Aug. 9, 2013, 5:23 p.m., Toshiaki Hatano wrote:
> > api/src/com/cloud/network/Networks.java, line 172
> > <https://reviews.apache.org/r/12849/diff/9/?file=338752#file338752line172>
> >
> > Why don't we merge getValueFrom() and getValue()?
> >
> > public String getValue(URI uri) {
> > if(uri.isOpaque()) {
> > return uri.getSchemeSpecificPart();
> > } else {
> > return uri.getHost();
> > }
> > }
>
> Toshiaki Hatano wrote:
> Or, maybe make getValueFrom private.
>
> I think it will make confuse if we provide 2 public method to get value of URI.
Sorry I missed the point, getValueFrom is static method.
Please disregard and close this issue.
- Toshiaki
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review24925
-----------------------------------------------------------
On Aug. 8, 2013, 11:51 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2013, 11:51 a.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.
>
> All over the code calls are done to URI.getHost() to retrieve ids of broadcastdomains. These id obviously are not hosts so this call is confusing and requires maintenance all over the code base. Also for different types the value returned by getHost has different meaning. vlan://1 is id 1 of course but others might be ranges of vlans or colon separated values. To make things worse a NiciraNvp has an uri of the form lswitch:<uuid> without the forward slashes.
>
> To make the system more maintainable in this perspect the changes in this patch were made. It is my intention to replace the calls to getHost by the member call getValueFrom or the static method getValue in time. In this way maintenance is centralized and an overview of differnces and quirks is easily found
>
>
> 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
> -------
>
> unit tests for different kind of BroadcastDomainType.values
>
>
> Thanks,
>
> daan Hoogland
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Toshiaki Hatano <to...@verio.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review24925
-----------------------------------------------------------
api/src/com/cloud/network/Networks.java
<https://reviews.apache.org/r/12849/#comment49071>
Why don't we merge getValueFrom() and getValue()?
public String getValue(URI uri) {
if(uri.isOpaque()) {
return uri.getSchemeSpecificPart();
} else {
return uri.getHost();
}
}
- Toshiaki Hatano
On Aug. 8, 2013, 11:51 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2013, 11:51 a.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.
>
> All over the code calls are done to URI.getHost() to retrieve ids of broadcastdomains. These id obviously are not hosts so this call is confusing and requires maintenance all over the code base. Also for different types the value returned by getHost has different meaning. vlan://1 is id 1 of course but others might be ranges of vlans or colon separated values. To make things worse a NiciraNvp has an uri of the form lswitch:<uuid> without the forward slashes.
>
> To make the system more maintainable in this perspect the changes in this patch were made. It is my intention to replace the calls to getHost by the member call getValueFrom or the static method getValue in time. In this way maintenance is centralized and an overview of differnces and quirks is easily found
>
>
> 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
> -------
>
> unit tests for different kind of BroadcastDomainType.values
>
>
> Thanks,
>
> daan Hoogland
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/
-----------------------------------------------------------
(Updated Aug. 8, 2013, 11:51 a.m.)
Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik Das, and Sheng Yang.
Changes
-------
extra description
Repository: cloudstack-git
Description (updated)
-------
Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility.
All over the code calls are done to URI.getHost() to retrieve ids of broadcastdomains. These id obviously are not hosts so this call is confusing and requires maintenance all over the code base. Also for different types the value returned by getHost has different meaning. vlan://1 is id 1 of course but others might be ranges of vlans or colon separated values. To make things worse a NiciraNvp has an uri of the form lswitch:<uuid> without the forward slashes.
To make the system more maintainable in this perspect the changes in this patch were made. It is my intention to replace the calls to getHost by the member call getValueFrom or the static method getValue in time. In this way maintenance is centralized and an overview of differnces and quirks is easily found
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 (updated)
-------
unit tests for different kind of BroadcastDomainType.values
Thanks,
daan Hoogland
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/
-----------------------------------------------------------
(Updated Aug. 8, 2013, 11:16 a.m.)
Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik Das, and Sheng Yang.
Changes
-------
making extra sure to remove as much objections as possible
Repository: cloudstack-git
Description
-------
Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
Diffs (updated)
-----
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
Re: Review Request 12849: added backwards compatibility code to
Networks enums
Posted by Daan Hoogland <da...@gmail.com>.
Chiradeep, Sheng,
Can you guys have a look at this? I cleaned up a bit with Davids guidance.
thanks,
Daan
On Wed, Aug 7, 2013 at 11:40 AM, daan Hoogland <da...@gmail.com>wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik
> Das, and Sheng Yang.
> By daan Hoogland.
>
> *Updated Aug. 7, 2013, 9:40 a.m.*
> Changes
>
> addressed minor issues brought up by David
>
> *Repository: * cloudstack-git
> Description
>
> Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
>
> Diffs (updated)
>
> - api/src/com/cloud/network/Networks.java (c76c3d4)
> - api/test/com/cloud/network/NetworksTest.java (31114e8)
>
> View Diff <https://reviews.apache.org/r/12849/diff/>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/
-----------------------------------------------------------
(Updated Aug. 7, 2013, 9:40 a.m.)
Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik Das, and Sheng Yang.
Changes
-------
addressed minor issues brought up by David
Repository: cloudstack-git
Description
-------
Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
Diffs (updated)
-----
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
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/
-----------------------------------------------------------
(Updated Aug. 7, 2013, 8:50 a.m.)
Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik Das, and Sheng Yang.
Changes
-------
did IsolationType forgot BroadcastDomainType. fixed
Repository: cloudstack-git
Description
-------
Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
Diffs (updated)
-----
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
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/
-----------------------------------------------------------
(Updated Aug. 7, 2013, 8:38 a.m.)
Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik Das, and Sheng Yang.
Changes
-------
string back in URI construction
Repository: cloudstack-git
Description
-------
Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
Diffs (updated)
-----
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
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
forgot to fix isolation type, fixed it
Repository: cloudstack-git
Description
-------
Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
Diffs (updated)
-----
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
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/
-----------------------------------------------------------
(Updated Aug. 6, 2013, 2:07 p.m.)
Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik Das, and Sheng Yang.
Changes
-------
default set to similar as before 4.1, but had to patch up to check for vlan as number or url
Repository: cloudstack-git
Description
-------
Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
Diffs (updated)
-----
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
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Dave Cahill <dc...@midokura.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review24720
-----------------------------------------------------------
As discussed on IRC Daan's aim is to allow using URIs of type lswitch:* rather than lswitch://*, so we agreed that existing code shouldn't be affected, and just lswitch behavior should be overridden.
Daan, thanks for the explanation, and look forward to seeing the new patch.
api/src/com/cloud/network/Networks.java
<https://reviews.apache.org/r/12849/#comment48824>
What I want is to have toUri work as before this commit, which broke backwards compatibility:
https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;a=blob;f=api/src/com/cloud/network/Networks.java;h=c76c3d4a473b5f87e8829aff1213b8d9be958190;hb=2d4464d2badc9aff842fd180bafc4c384a83a91d
i.e.
return new URI(scheme + "://" + value);
Not this:
return new URI(value.toString());
- Dave Cahill
On Aug. 6, 2013, 10:41 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2013, 10:41 a.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
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/
-----------------------------------------------------------
(Updated Aug. 6, 2013, 10:41 a.m.)
Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik Das, and Sheng Yang.
Changes
-------
default behavior as decided after consult with Dave
Repository: cloudstack-git
Description
-------
Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
Diffs (updated)
-----
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
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by Dave Cahill <dc...@midokura.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/#review24710
-----------------------------------------------------------
api/src/com/cloud/network/Networks.java
<https://reviews.apache.org/r/12849/#comment48820>
Commenting as discussed on list - I think we should remove the check for contains(":") at this line. We weren't doing it before July 20, it's invalid as per the URI spec, and it breaks existing callers.
- Dave Cahill
On Aug. 6, 2013, 9:16 a.m., daan Hoogland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12849/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2013, 9:16 a.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
>
>
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/
-----------------------------------------------------------
(Updated Aug. 6, 2013, 9:16 a.m.)
Review request for cloudstack, Chiradeep Vittal, Dave Cahill, Koushik Das, and Sheng Yang.
Changes
-------
get Dave involved, because of his mido code
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
Re: Review Request 12849: added backwards compatibility code to Networks
enums
Posted by daan Hoogland <da...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12849/
-----------------------------------------------------------
(Updated Aug. 5, 2013, 9:57 a.m.)
Review request for cloudstack, Chiradeep Vittal, Koushik Das, and Sheng Yang.
Changes
-------
A more permanent take on toUri, using member-methods per value.
Repository: cloudstack-git
Description
-------
Both BroadcastDomainType and IsolationType needed some extra code for backwards compatibility
Diffs (updated)
-----
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