Re: [Crucible] State changed to Review: (CR-MOD-416) ATOM-7 - Implement AtomFeedUtil.getAtomFeedStream

classic Classic list List threaded Threaded
5 messages Options
Darius Jazayeri-4 Darius Jazayeri-4
Reply | Threaded
Open this post in threaded view
|

Re: [Crucible] State changed to Review: (CR-MOD-416) ATOM-7 - Implement AtomFeedUtil.getAtomFeedStream

Ben,

I just peeked at this review when I saw it in my inbox. Maybe you're dealing with threading and synchronization elsewhere, I haven't looked.

I'm a bit worried that we're giving static access to getting a stream to the file that stores the atom feed in a Util method.

Don't we want this to be in the service layer, and don't we want to ensure that writing a new entry to that file is a synchronized, atomic operation?

-Darius

On Mon, May 7, 2012 at 10:19 AM, Mykola Vorobey <[hidden email]> wrote:
Crucible
Review ATOM-7 - Implement AtomFeedUtil.getAtomFeedStream
CR-MOD-416
The state of CR-MOD-416 has been changed:
  • Mykola Vorobey changed it to Review
Participants
Mykola Vorobey Author
Darius Jazayeri Reviewer
Ben Wolfe Reviewer
Sent by Atlassian Crucible 2.7.1


[hidden email] from OpenMRS Developers' mailing list
Ben Wolfe (openmrs) Ben Wolfe (openmrs)
Reply | Threaded
Open this post in threaded view
|

Re: [Crucible] State changed to Review: (CR-MOD-416) ATOM-7 - Implement AtomFeedUtil.getAtomFeedStream

Threading is dealt with by activemq.

This method does need to be synchronized. But why does it need to be in a service layer? No db transaction is occurring.

Ben

On May 7, 2012 7:09 PM, "Darius Jazayeri" <[hidden email]> wrote:
Ben,

I just peeked at this review when I saw it in my inbox. Maybe you're dealing with threading and synchronization elsewhere, I haven't looked.

I'm a bit worried that we're giving static access to getting a stream to the file that stores the atom feed in a Util method.

Don't we want this to be in the service layer, and don't we want to ensure that writing a new entry to that file is a synchronized, atomic operation?

-Darius

On Mon, May 7, 2012 at 10:19 AM, Mykola Vorobey <[hidden email]> wrote:
Crucible
Review ATOM-7 - Implement AtomFeedUtil.getAtomFeedStream
CR-MOD-416
The state of CR-MOD-416 has been changed:
  • Mykola Vorobey changed it to Review
Participants
Mykola Vorobey Author
Darius Jazayeri Reviewer
Ben Wolfe Reviewer
Sent by Atlassian Crucible 2.7.1


[hidden email] from OpenMRS Developers' mailing list

[hidden email] from OpenMRS Developers' mailing list
Darius Jazayeri-2 Darius Jazayeri-2
Reply | Threaded
Open this post in threaded view
|

Re: [Crucible] State changed to Review: (CR-MOD-416) ATOM-7 - Implement AtomFeedUtil.getAtomFeedStream

I meant that the method should not be publicly accessible. The service should only expose a single method that adds a feed item to the file.

-Darius (by phone)

On May 7, 2012 7:20 PM, "Ben Wolfe" <[hidden email]> wrote:

Threading is dealt with by activemq.

This method does need to be synchronized. But why does it need to be in a service layer? No db transaction is occurring.

Ben

On May 7, 2012 7:09 PM, "Darius Jazayeri" <[hidden email]> wrote:
Ben,

I just peeked at this review when I saw it in my inbox. Maybe you're dealing with threading and synchronization elsewhere, I haven't looked.

I'm a bit worried that we're giving static access to getting a stream to the file that stores the atom feed in a Util method.

Don't we want this to be in the service layer, and don't we want to ensure that writing a new entry to that file is a synchronized, atomic operation?

-Darius

On Mon, May 7, 2012 at 10:19 AM, Mykola Vorobey <[hidden email]> wrote:
Crucible
Review ATOM-7 - Implement AtomFeedUtil.getAtomFeedStream
CR-MOD-416
The state of CR-MOD-416 has been changed:
  • Mykola Vorobey changed it to Review
Participants
Mykola Vorobey Author
Darius Jazayeri Reviewer
Ben Wolfe Reviewer
Sent by Atlassian Crucible 2.7.1


[hidden email] from OpenMRS Developers' mailing list

[hidden email] from OpenMRS Developers' mailing list

[hidden email] from OpenMRS Developers' mailing list
Ben Wolfe (openmrs) Ben Wolfe (openmrs)
Reply | Threaded
Open this post in threaded view
|

Re: [Crucible] State changed to Review: (CR-MOD-416) ATOM-7 - Implement AtomFeedUtil.getAtomFeedStream

Ah, ok, I see what you mean now and I totally agree!  We do only have one method to write to the feed, and that will be synchronized: AtomFeedUtil.writeToFeed(action, OpenmrsObject)

This method you are looking at is: AtomFeedUtil.getAtomFeedStream(). It gets an OutputStream for the servlet to display the feed.  It has to be public.  Perhaps we should rename it to be more obvious that you are getting the stream for output purposes and not input purposes?

Ben

On Tue, May 8, 2012 at 12:07 AM, Darius Jazayeri <[hidden email]> wrote:

I meant that the method should not be publicly accessible. The service should only expose a single method that adds a feed item to the file.

-Darius (by phone)

On May 7, 2012 7:20 PM, "Ben Wolfe" <[hidden email]> wrote:

Threading is dealt with by activemq.

This method does need to be synchronized. But why does it need to be in a service layer? No db transaction is occurring.

Ben

On May 7, 2012 7:09 PM, "Darius Jazayeri" <[hidden email]> wrote:
Ben,

I just peeked at this review when I saw it in my inbox. Maybe you're dealing with threading and synchronization elsewhere, I haven't looked.

I'm a bit worried that we're giving static access to getting a stream to the file that stores the atom feed in a Util method.

Don't we want this to be in the service layer, and don't we want to ensure that writing a new entry to that file is a synchronized, atomic operation?

-Darius

On Mon, May 7, 2012 at 10:19 AM, Mykola Vorobey <[hidden email]> wrote:
Crucible
Review ATOM-7 - Implement AtomFeedUtil.getAtomFeedStream
CR-MOD-416
The state of CR-MOD-416 has been changed:
  • Mykola Vorobey changed it to Review
Participants
Mykola Vorobey Author
Darius Jazayeri Reviewer
Ben Wolfe Reviewer
Sent by Atlassian Crucible 2.7.1


[hidden email] from OpenMRS Developers' mailing list

[hidden email] from OpenMRS Developers' mailing list

[hidden email] from OpenMRS Developers' mailing list


[hidden email] from OpenMRS Developers' mailing list
Darius Jazayeri-3 Darius Jazayeri-3
Reply | Threaded
Open this post in threaded view
|

Re: [Crucible] State changed to Review: (CR-MOD-416) ATOM-7 - Implement AtomFeedUtil.getAtomFeedStream

Yes, that's it, at a quick glance I figured that the method that got a stream was for writing, not reading.

-Darius

On Tue, May 8, 2012 at 6:28 AM, Ben Wolfe <[hidden email]> wrote:
Ah, ok, I see what you mean now and I totally agree!  We do only have one method to write to the feed, and that will be synchronized: AtomFeedUtil.writeToFeed(action, OpenmrsObject)

This method you are looking at is: AtomFeedUtil.getAtomFeedStream(). It gets an OutputStream for the servlet to display the feed.  It has to be public.  Perhaps we should rename it to be more obvious that you are getting the stream for output purposes and not input purposes?

Ben


On Tue, May 8, 2012 at 12:07 AM, Darius Jazayeri <[hidden email]> wrote:

I meant that the method should not be publicly accessible. The service should only expose a single method that adds a feed item to the file.

-Darius (by phone)

On May 7, 2012 7:20 PM, "Ben Wolfe" <[hidden email]> wrote:

Threading is dealt with by activemq.

This method does need to be synchronized. But why does it need to be in a service layer? No db transaction is occurring.

Ben

On May 7, 2012 7:09 PM, "Darius Jazayeri" <[hidden email]> wrote:
Ben,

I just peeked at this review when I saw it in my inbox. Maybe you're dealing with threading and synchronization elsewhere, I haven't looked.

I'm a bit worried that we're giving static access to getting a stream to the file that stores the atom feed in a Util method.

Don't we want this to be in the service layer, and don't we want to ensure that writing a new entry to that file is a synchronized, atomic operation?

-Darius

On Mon, May 7, 2012 at 10:19 AM, Mykola Vorobey <[hidden email]> wrote:
Crucible
Review ATOM-7 - Implement AtomFeedUtil.getAtomFeedStream
CR-MOD-416
The state of CR-MOD-416 has been changed:
  • Mykola Vorobey changed it to Review
Participants
Mykola Vorobey Author
Darius Jazayeri Reviewer
Ben Wolfe Reviewer
Sent by Atlassian Crucible 2.7.1


[hidden email] from OpenMRS Developers' mailing list

[hidden email] from OpenMRS Developers' mailing list

[hidden email] from OpenMRS Developers' mailing list


[hidden email] from OpenMRS Developers' mailing list


[hidden email] from OpenMRS Developers' mailing list