I'm not really sure why you think it is a bug. And why do you want to send data that is absolutely useless to the destination?
Thanks Alex Ough On Mon, May 12, 2014 at 6:19 PM, Alena Prokharchyk < alena.prokharc...@citrix.com> wrote: > Alex, I can’t approve the current approach you use in your fix. The > reason that there are bugs in the current CS code, and therefore we can > contribute more to the buggy behavior, doesn’t sound right to me. And we > have –1 from Alex Huang on that as well. > > We either fix it as a part of this commit, or you can fix it later. But > it has to make it to 4.5, otherwise the original fix will be rolled back. > You can finalize the approach with Alex, and I will check in your code as > soon as its done, either before I go on vacation, or after I’m back. > > -Alena. > > From: Alex Ough <alex.o...@sungardas.com> > Date: Monday, May 12, 2014 at 3:13 PM > To: Alena Prokharchyk <alena.prokharc...@citrix.com> > Cc: Alex Huang <alex.hu...@citrix.com>, Murali Reddy < > murali.re...@citrix.com>, Kishan Kavala <kishan.kav...@citrix.com>, " > dev@cloudstack.apache.org" <dev@cloudstack.apache.org> > > Subject: Re: Control event publishing in multi region setups > > That is not good, but I'm wondering if you can approve after our > conversation without consulting with Alex Hwang. > > Thanks > Alex Ough > > > On Mon, May 12, 2014 at 2:37 PM, Alena Prokharchyk < > alena.prokharc...@citrix.com> wrote: > >> We do have to come to conclusion for this remaining issue before >> committing the patches below: >> (https://reviews.apache.org/r/20099/ and >> https://reviews.apache.org/r/17790/) >> >> Alex (Ough), I’m going to be on vacation from May 15th till May 31st, >> if you and Alex(Huang) have your discussion/resolution while I’m away, I >> can commit the patches only after I’m back. >> >> Thank you! >> Alena. >> >> From: Alex Ough <alex.o...@sungardas.com> >> Date: Sunday, May 11, 2014 at 10:10 PM >> To: Alex Huang <alex.hu...@citrix.com> >> Cc: Murali Reddy <murali.re...@citrix.com>, Alena Prokharchyk < >> alena.prokharc...@citrix.com>, Kishan Kavala <kishan.kav...@citrix.com>, >> "dev@cloudstack.apache.org" <dev@cloudstack.apache.org> >> >> Subject: Re: Control event publishing in multi region setups >> >> Alex, >> >> It looks like I'd better wait until you're back because I'm afraid >> Alena seems to need your approval based on what I've been through. >> Let me know once you're back. >> >> Thanks >> Alex Ough >> >> >> On Sat, May 10, 2014 at 12:50 PM, Alex Huang <alex.hu...@citrix.com>wrote: >> >>> Alex and Alena, >>> >>> >>> >>> Perhaps, it’s best you two get on the phone about this. I don’t see >>> Alex understanding what I’m saying over email so there’s no point in me >>> repeating it. I’m not around next week and I think Alena is out after >>> Wednesday. Something on Monday or Tuesday would be a good idea or you can >>> wait for me to come back the week after. >>> >>> >>> >>> --Alex >>> >>> >>> >>> *From:* Alex Ough [mailto:alex.o...@sungardas.com] >>> *Sent:* Saturday, May 10, 2014 9:28 AM >>> *To:* Alex Huang >>> >>> *Cc:* Murali Reddy; Alena Prokharchyk; Kishan Kavala; >>> dev@cloudstack.apache.org >>> *Subject:* Re: Control event publishing in multi region setups >>> >>> >>> >>> And I'm really wondering if you understood how the 'Full Scan' works. It >>> is absolutely internal operations. >>> >>> Why do we force to use the event generating methods when the updates are >>> only internal and never, ever, ever ... need events? >>> >>> >>> >>> Let me know if there is any chance it needs to use the events, then I'll >>> follow your suggestion. >>> >>> Thanks >>> >>> Alex Ough >>> >>> >>> >>> On Sat, May 10, 2014 at 11:55 AM, Alex Ough <alex.o...@sungardas.com> >>> wrote: >>> >>> I really don't know why you guys are making it complicated. >>> >>> The class has two different methods, one with 'event' decorator and the >>> other without it. >>> >>> So the callers know which method to call depending on their needs. >>> >>> And the each method will be called accordingly. >>> >>> >>> >>> On Sat, May 10, 2014 at 6:13 AM, Alex Huang <alex.hu...@citrix.com> >>> wrote: >>> >>> -1 >>> >>> >>> >>> I do not believe in the argument that says “since there’s existing bad >>> code, then I can check in code that also causes regressions for users.” If >>> that’s the case, what’s the point of the review? >>> >>> >>> >>> We’ve offered a path forward already. Please reconsider that. >>> >>> >>> >>> --Alex >>> >>> >>> >>> *From:* Alex Ough [mailto:alex.o...@sungardas.com] >>> *Sent:* Friday, May 9, 2014 9:14 PM >>> *To:* Alex Huang >>> *Cc:* Murali Reddy; Alena Prokharchyk; Kishan Kavala; >>> dev@cloudstack.apache.org >>> >>> >>> *Subject:* Re: Control event publishing in multi region setups >>> >>> >>> >>> Are we going to rolling this out? >>> >>> >>> >>> On Thu, May 8, 2014 at 2:28 PM, Alex Ough <alex.o...@sungardas.com> >>> wrote: >>> >>> That's why there are 2 methods, one is that generates events and the >>> other not and there are already a few public methods without event >>> decoration. >>> >>> >>> >>> On Thu, May 8, 2014 at 2:25 PM, Alex Huang <alex.hu...@citrix.com> >>> wrote: >>> >>> Alex, >>> >>> >>> >>> I did read this when you first proposed. I do understand the two >>> implementation. >>> >>> >>> >>> I understand that #2 is not activated via events but it doesn’t mean #2 >>> can just don’t generate events. The blocker is precisely with the last >>> sentence in #2 where it states #2 doesn’t generate an event when “it >>> creates/updates/removes the resource in the local region”. >>> >>> >>> >>> Perhaps an example would make this more clear. >>> >>> >>> >>> Someone who deploys CloudStack sets up a process to listen to account >>> events. It is a simple audit process whose job is to verify that an >>> account created in CloudStack is actually in their own billing database. >>> The fact that #2 doesn’t generate an event would mean this process would >>> be broken for them. This is the regression that causes the blocker. >>> >>> >>> >>> --Alex >>> >>> >>> >>> >>> >>> *From:* Alex Ough [mailto:alex.o...@sungardas.com] >>> *Sent:* Thursday, May 8, 2014 11:02 AM >>> *To:* Alex Huang >>> *Cc:* Murali Reddy; Alena Prokharchyk; Kishan Kavala >>> >>> >>> *Subject:* Re: Control event publishing in multi region setups >>> >>> >>> >>> Alex, >>> >>> >>> >>> I think you really review the wiki ( >>> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Domain-Account-User+Sync+Up+Among+Multiple+Regions) >>> or the implemented codes. >>> >>> >>> >>> To help you understand, there are 2 synchronizations supported in this >>> feature. >>> >>> >>> >>> 1. real time sync : This is what you may imagine and event based. This >>> is sending requests when they are created/updated/removed in the local >>> region by subscribing their events. >>> >>> >>> >>> 2. full scan : This is NOT related with events and it is to cover when >>> the #1 sync is failed with any reason like network failures. With interval, >>> it just scans all resources and compare them with ones in remote regions >>> and if there is any missing in the local region, it creates/updates/removes >>> the resource in the local region and the NEW METHODS I need are called >>> because it is local region only and no need to have events. >>> >>> >>> >>> I'm hoping you understand the feature a little more and let me know if >>> you need more information. >>> >>> Thanks >>> >>> Alex Ough >>> >>> >>> >>> >>> >>> On Thu, May 8, 2014 at 1:43 PM, Alex Huang <alex.hu...@citrix.com> >>> wrote: >>> >>> Hi Alex, >>> >>> >>> >>> Please know that the contribution is much appreciated. It is not a case >>> of whether or not Alena “wants” or “doesn’t want” to approve the review. >>> She can only approve if the design is sound and has no regression for >>> existing deployments of CloudStack. >>> >>> >>> >>> This is a blocker because not publishing events when an account is >>> propagated is actually an “incorrect” behavior for CloudStack. Any >>> functionality that acts on an account creation within the region will face >>> regression. That’s why it is not “an additional feature” and must be >>> fixed. Think of SunGuard itself. If it was depending on the account >>> creation event and the next version of CloudStack suddenly doesn’t generate >>> the event consistently, would it not consider this a bug and ask us to fix >>> it? >>> >>> >>> >>> I do understand the time consuming nature of providing patches and >>> merging code. Alena tells me that she has reviewed the code and she thinks >>> the design is fine except for this one item. If we can commit to fix this >>> problem after the code is checked in, we can check it in now just so you >>> don’t have to do another round of merge and review for the part that is >>> working. But the fix will need to be in before the code is released or >>> else we might have to revert this checkin. What do you think? >>> >>> >>> >>> --Alex >>> >>> P.S. I’m not sure why this is not on the dev list. We should bring this >>> back. >>> >>> >>> >>> *From:* Alex Ough [mailto:alex.o...@sungardas.com] >>> *Sent:* Wednesday, May 7, 2014 4:58 PM >>> *To:* Murali Reddy >>> *Cc:* Alena Prokharchyk; Alex Huang; Kishan Kavala >>> >>> >>> *Subject:* Re: Control event publishing in multi region setups >>> >>> >>> >>> All, >>> >>> >>> >>> Alena doesn't want to approve my implementation because of this email >>> thread, but I'm frustrated and not sure why this is a blocker. >>> >>> What I did was just created another method without an event tag like the >>> one already existing in 'AccountManagerImpl' class as below. >>> >>> >>> >>> @Override >>> >>> public boolean enableAccount(long accountId) >>> >>> >>> >>> And if we need this feature, we really need to create a new jira instead >>> of adding it to already existing one >>> >>> so that we can discuss options to find a best solution. >>> >>> >>> >>> It's been a really long path mostly because of miscommunications, and I >>> really want to wrap this up without adding a new feature that is not >>> existing. >>> >>> >>> >>> Let me know what you think. >>> >>> Thanks >>> >>> Alex Ough >>> >>> >>> >>> On Wed, May 7, 2014 at 10:29 AM, Murali Reddy <murali.re...@citrix.com> >>> wrote: >>> >>> I don’t think we need to bring back reverted changes, as we want all >>> the events generated should be published all the time with in the region. I >>> agree with Alex Huang, that we could actually add details (originating >>> region) to the account indicating source region where account is created. >>> Details particular to an event published on the event bus is a JSON object >>> so we can add additional details. Also steps listed out by Alex should >>> prevent from cyclic propagation. >>> >>> >>> >>> Alex Ough, >>> >>> >>> >>> Suggested steps below by alex should work for you. Do you see any >>> problem with that? >>> >>> >>> >>> Thanks, >>> >>> Murali >>> >>> >>> >>> *From: *Alena Prokharchyk <alena.prokharc...@citrix.com> >>> *Date: *Wednesday, 7 May 2014 5:56 AM >>> *To: *Alex Huang <alex.hu...@citrix.com>, Alex Ough < >>> alex.o...@sungardas.com>, Kishan Kavala <kishan.kav...@citrix.com>, >>> Murali Reddy <murali.re...@citrix.com> >>> >>> >>> *Subject: *Re: Control event publishing in multi region setups >>> >>> >>> >>> Alex (Huang), thanks for commenting. As a conclusion – we should never >>> omit event firing when submit create/update. >>> >>> >>> >>> >>> >>> Kishan/Murali, can you please help Alex (Ough) to figure out how to >>> implement the behavior Kishan reverted. Kishan, can you check with Murali >>> how to bring back your reverted changes for the API to make it work with >>> the new events framework? >>> >>> >>> >>> Thank you, >>> >>> Alena. >>> >>> *From: *Alex Huang <alex.hu...@citrix.com> >>> *Date: *Tuesday, May 6, 2014 at 10:17 AM >>> *To: *Alena Prokharchyk <alena.prokharc...@citrix.com>, Alex Ough < >>> alex.o...@sungardas.com> >>> *Cc: *Kishan Kavala <kishan.kav...@citrix.com> >>> *Subject: *RE: Control event publishing in multi region setups >>> >>> >>> >>> Hey guys, >>> >>> >>> >>> I’m not sure we’re all on the same page. >>> >>> >>> >>> First, the event must always be published, regardless of if it was >>> propagated from another region or created originally in that region. The >>> reason is there may be other code interested in acting on account creation >>> in a region. We just need to provide a way for Alex’s code to determine >>> that the account is propagated rather than created originally in the >>> region. You don’t need details in the event for that. >>> >>> >>> >>> The propagation code can do the following. It’s probably already doing >>> that. >>> >>> >>> >>> 1. Listen for the account creation event. >>> >>> 2. Upon receiving an account creation event, retrieve the account >>> to check if the account is propagated or created. >>> >>> 3. If propagated, then don’t propagate or maybe even signal back >>> that the propagation is done for this region (depending on the propagation >>> logic). If created, then propagate to other regions. >>> >>> >>> >>> Now the detail is in how do we know if an account is created or >>> propagated. For that, it can be done in a number of ways. I’m open to >>> which method. I would suggest that we add two fields to account: >>> origination region and original uuid. The create account API takes an >>> optional fields for the origination region and origination account uuid. >>> If these two parameters are not set in the API, the API set the >>> origination region to the current region and the original uuid to the uuid >>> of the account. >>> >>> >>> >>> Sorry for the confusion here. I had thought Kishan added this but >>> apparently it has been reverted. >>> >>> >>> >>> --Alex >>> >>> >>> >>> *From:* Alena Prokharchyk >>> *Sent:* Tuesday, May 6, 2014 9:57 AM >>> *To:* Alex Ough >>> *Cc:* Kishan Kavala; Alex Huang >>> *Subject:* Re: Control event publishing in multi region setups >>> >>> >>> >>> Ok, thank you Alex, so looks like there is no other workaround as of now >>> rather than introducing the new methods to the managers. Just go ahead and >>> submit the rest of the fixes for both review tickets, and I will commit the >>> patch after that. >>> >>> >>> >>> -Alena. >>> >>> >>> >>> *From: *Alex Ough <alex.o...@sungardas.com> >>> *Date: *Tuesday, May 6, 2014 at 9:48 AM >>> *To: *Alena Prokharchyk <alena.prokharc...@citrix.com> >>> *Cc: *Kishan Kavala <kishan.kav...@citrix.com>, Alex Huang < >>> alex.hu...@citrix.com> >>> *Subject: *Re: Control event publishing in multi region setups >>> >>> >>> >>> I'm afraid it is not possible because the events are set in the method >>> level and one of my colleagues implemented to enable/disable events, but >>> this is working as globally. >>> >>> >>> >>> On Tue, May 6, 2014 at 12:44 PM, Alena Prokharchyk < >>> alena.prokharc...@citrix.com> wrote: >>> >>> Kishan, any updates from Murali? All we need to know is – if >>> controlling events possible when command is fired through CS client APIs >>> today. >>> >>> >>> >>> Thank you! >>> >>> Alena. >>> >>> >>> >>> *From: *Kishan Kavala <kishan.kav...@citrix.com> >>> *Date: *Tuesday, May 6, 2014 at 7:22 AM >>> *To: *Alena Prokharchyk <alena.prokharc...@citrix.com> >>> *Cc: *Alex Ough <alex.o...@sungardas.com>, Alex Huang < >>> alex.hu...@citrix.com> >>> >>> >>> *Subject: *Re: Control event publishing in multi region setups >>> >>> >>> >>> Alena, >>> >>> Events are published using the event framework introduced by Murali. It >>> can contain additional details to indicate whether an event should be >>> propagated to other regions. >>> >>> In the implementation I added using API sync, there was a flag in the >>> API params to indicate whether to propagate event or not. I reverted this >>> code later when we moved to use event framework. >>> >>> I'll check with Murali for more details regarding adding custom >>> details / extending event details. >>> >>> >>> On 06-May-2014, at 4:52 am, "Alena Prokharchyk" < >>> alena.prokharc...@citrix.com> wrote: >>> >>> Alex, I understand that. But if Kishan implemented the way of >>> extending the events with the details that can be later on read by events >>> recipient, then you should be able to use the API. >>> >>> >>> >>> If there is no such support, the I agree that the way you do it now, is >>> the only one way to achieve the desired functionality. >>> >>> >>> >>> -Alena. >>> >>> >>> >>> *From: *Alex Ough <alex.o...@sungardas.com> >>> *Date: *Monday, May 5, 2014 at 4:08 PM >>> *To: *Alex Huang <alex.hu...@citrix.com> >>> *Cc: *Alena Prokharchyk <alena.prokharc...@citrix.com>, Kishan Kavala < >>> kishan.kav...@citrix.com> >>> *Subject: *Re: Control event publishing in multi region setups >>> >>> >>> >>> That's exactly why I need methods that do NOT generate events when the >>> create/update/delete is just for local resources. >>> >>> >>> >>> On Mon, May 5, 2014 at 7:06 PM, Alex Huang <alex.hu...@citrix.com> >>> wrote: >>> >>> That’s actually what I said. Let me clarify. When Kishan added the >>> region feature, we discussed the problem of infinite circular propagation >>> because each management server that adds an account will attempt to >>> propagate it to all the regions by adding it to that region and so on. The >>> API needs provide a way for that propagation to be terminated. That >>> doesn’t mean we don’t publish the event in the region where the account is >>> propagated to. We still should publish the event because that region did >>> add a new account but the event needs to contain enough details for anyone >>> listening to the event to determine that they should not propagate the >>> account creation. >>> >>> >>> >>> --Alex >>> >>> >>> >>> *From:* Alena Prokharchyk >>> *Sent:* Monday, May 5, 2014 2:39 PM >>> *To:* Kishan Kavala; Alex Ough >>> *Cc:* Alex Huang >>> *Subject:* Control event publishing in multi region setups >>> >>> >>> >>> Kishan, >>> >>> >>> >>> Have a question to you. Alex Huang mentioned to me that you were >>> planning to add support for controlling event publishing in multi regions >>> setup. So you can control whether you want to publish the event in a >>> particular region when create/update/delete account/domain API call is >>> made. Can you please tell us if you’ve implemented it? And what parameters >>> need to be passed to the API call to achieve that. >>> >>> >>> >>> Alex (Ought), if Kishan didn’t implement this, then I agree with the way >>> you’ve added new methods to Account/DomainManagers to do the object update >>> w/o the event generation. Lets wait for Kishan’s reply. By now, you can go >>> ahead and fix 1) and 2) in https://reviews.apache.org/r/20099/ which is >>> not related to event generation. >>> >>> >>> >>> Thank you! >>> >>> -Alena. >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >> >> >
