Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code for Issues #301 & #112 (dev pull) #327

Closed
wants to merge 3 commits into from
Closed

Code for Issues #301 & #112 (dev pull) #327

wants to merge 3 commits into from

Conversation

BillAtPros
Copy link

API returns HTTP Status Code 204 when Queue is empty. In this case
calling getValue() on the ReceiveQueueMessageResult will return null.

Changed ServiceBusRestProxy:receiveMessage() now returns a null BrokeredMessage from the getValue() call in cases where the REST call returns a 204 Status Code, which indicates the Queue is empty and remained empty until recieveMessage() call times out.

Tests added in ServiceBusIntegrationTest to test empty Queues in both Receive/Delete and Peek/Lock scenarios. Updated peekLockedMessageCanBeDeleted() to function with null BrokeredMessage returned in empty Queue scenario.

retrieving Message on Empty Queue
Add test cases when Retrieving from Empty Queue in both RECEIVE/DELETE
and PEEK/LOCK modes.
Update existing test to be compatible with receiving null
BrokeredMessage instead of uninitialized one.
@BillAtPros
Copy link
Author

new pull request from dev (instead of master)

@gcheng
Copy link

gcheng commented Apr 16, 2013

@BillAtPros have u signed the CLA?

@BillAtPros
Copy link
Author

YES.

See guangyang's last comment here

@gcheng
Copy link

gcheng commented Apr 16, 2013

gr8!

@@ -183,6 +183,11 @@ else if (options.isPeekLock()) {
throw new RuntimeException("Unknown ReceiveMode");
}

//Empty Queue scenario
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to MSDN, 204 means "No messages available within the specified timeout period.". If we document, it will be good to use this text. 204 could mean the queue is empty, but it can also mean there are message in the queue but locked. http://msdn.microsoft.com/en-us/library/windowsazure/hh780735.aspx

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point and agreed.How bout...
\No Messages Available scenario
This covers Empty as well as Messages Present but Locked.
We may also want to add a unit test to cover the Messages Present but Locked case.

@gcheng
Copy link

gcheng commented Apr 17, 2013

One comment left, the rest LGTM.

@BillAtPros
Copy link
Author

Responded to your comment. Let me know if you want me to make the changes and resubmit the pull.

@gcheng
Copy link

gcheng commented Apr 18, 2013

Thanks @BillAtPros , I think you can just made the change, push the change to BillAtPros:dev, afterwards, the pull request will be updated magically by github.

wdixon added 2 commits April 18, 2013 13:42
retrieving Message on Empty Queue
Add test cases when Retrieving from Empty Queue in both RECEIVE/DELETE
and PEEK/LOCK modes.
Update existing test to be compatible with receiving null
BrokeredMessage instead of uninitialized one.
https://github.com/BillAtPros/azure-sdk-for-java.git into dev

Conflicts:
	microsoft-azure-api/src/main/java/com/microsoft/windowsazure/services/serviceBus/implementation/ServiceBusRestProxy.java
@BillAtPros
Copy link
Author

I've really jacked up my repository during a merge (which I can't understand why it was required). cf7b4b6 looks good. Pull ASAP so I can kill my fork. I've spent way too much time jacking with this.

Thanks

@gcheng
Copy link

gcheng commented Apr 18, 2013

Hi @BillAtPros , it seems you are also changling line endings of a lot of files. Those need to be reverted. It seems now 675 files are changed. Sorry about that, but otherwise it will make other developers to revert those unexpected changes. Thanks for understanding.

@BillAtPros
Copy link
Author

I'm aware. That's why I asked you to pull changeset cf7b4b6, which is fine, instead of the later e81332d in the comment previous to yours.

@gcheng
Copy link

gcheng commented Apr 19, 2013

can we revert e81332d?

@BillAtPros
Copy link
Author

I've been trying off and on for hours. I'm a git newbie and have a day job. If you can revert it, go crazy.

Otherwise, I'll probably end up deleting my fork and doing the changes AGAIN, rather than struggling with git for another 5 hours.

@christav
Copy link
Contributor

I can help with reverting if you'd like.

@BillAtPros
Copy link
Author

it would be much appreciated

@gcheng
Copy link

gcheng commented Apr 19, 2013

No worry, I will take care of it this time.

@BillAtPros
Copy link
Author

Much appreciate. I'll do better next time, I promise.

@christav
Copy link
Contributor

@BillAtPros, I just sent you a PR against your branch which should detangle your repo and fix this PR. Please let me know if it works. You should be able to merge it and then this PR will be automatically updated. You should probably test it first though in case I grabbed the wrong side of the merge.

@gcheng
Copy link

gcheng commented Apr 19, 2013

@christav I am taking care of it.

@gcheng
Copy link

gcheng commented Apr 22, 2013

a separated pull request from me had addressed this issue. close this one. 2fe266a

@gcheng gcheng closed this Apr 22, 2013
mssfang pushed a commit to mssfang/azure-sdk-for-java that referenced this pull request Apr 23, 2019
navalev pushed a commit to navalev/azure-sdk-for-java that referenced this pull request Dec 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants