Implement SortableBlockingQueue - [merged] #152

Closed jdonkervliet closed 4 years ago

In GitLab by @swabbur on May 25, 2020, 12:19

Merges feature/custom-queue -> development

Add a sortable blocking queue for use in chunk streaming.

In GitLab by @larsdetombe on May 25, 2020, 12:21

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 109

Can you do the notNull checks in the code blocks instead?

In GitLab by @swabbur on May 25, 2020, 12:30

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 109

These annotations were added automatically by Intellij code generation. I personal prefer them over manual checks (as the annotations check the code while writing instead of at runtime).

In GitLab by @larsdetombe on May 25, 2020, 12:35

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 212

Do you still want this timeout, or is this something for the future?

In GitLab by @larsdetombe on May 25, 2020, 12:38

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 326

Does this require correct ordering?

In GitLab by @JimVliet on May 25, 2020, 12:42

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 37

Could you rename the comparator field to signify that this comparator is actually the reverse of the comparator that was supplied by the caller.

In GitLab by @wubero on May 25, 2020, 12:43

Commented on src/test/java/net/glowstone/util/SortableBlockingQueueTest.java line 3

I wouldn't use a star import here. Not really a big deal but might be better if we don't use it

In GitLab by @swabbur on May 25, 2020, 12:43

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 212

Yes, I'll add it in now. I was doubting about the implementation as the timeout would be reset when the thread is signaled.

In GitLab by @swabbur on May 25, 2020, 12:44

Commented on src/test/java/net/glowstone/util/SortableBlockingQueueTest.java line 3

Missed that one, it was automatically generated by Intellij on creating the test file.

In GitLab by @swabbur on May 25, 2020, 12:44

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 326

I am not sure, I'll look it up.

In GitLab by @swabbur on May 25, 2020, 12:48

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 326

It is never explicitly stated in the documentation, but I'll make it ordered just to be sure.

In GitLab by @swabbur on May 25, 2020, 13:02

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 212

changed this line in version 2 of the diff

In GitLab by @swabbur on May 25, 2020, 13:02

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 37

changed this line in version 2 of the diff

In GitLab by @swabbur on May 25, 2020, 13:02

Commented on src/test/java/net/glowstone/util/SortableBlockingQueueTest.java line 3

changed this line in version 2 of the diff

In GitLab by @swabbur on May 25, 2020, 13:02

added 4 commits

e14cc9e6 - Remove unnecessary star-import
0b47c3d2 - Add timeout to poll method
3a24ed5a - Rename comparator to reverseComparator
9f0ddae8 - Fix drainTo element order

Compare with previous version

In GitLab by @swabbur on May 25, 2020, 13:04

added 1 commit

99a7386d - Fix drainTo concurrency issue

Compare with previous version

In GitLab by @JimVliet on May 25, 2020, 13:20

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 20

The javadocs should properly reflect the fact that it gives different guarantees than you would normally expect when using a blocking queue.

In GitLab by @swabbur on May 25, 2020, 13:25

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 20

changed this line in version 4 of the diff

In GitLab by @swabbur on May 25, 2020, 13:25

added 1 commit

efb0df06 - Clarify the order guarantee provided by the queue

Compare with previous version

In GitLab by @swabbur on May 25, 2020, 13:26

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 20

I've added a line explaining that order is not guaranteed when insertions are performed in between sort calls.

In GitLab by @wubero on May 25, 2020, 13:50

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 192

Can't you just call peek() here?

In GitLab by @wubero on May 25, 2020, 13:50

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 224

Can't you just call element() here?

In GitLab by @swabbur on May 25, 2020, 13:52

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 192

No, peek only retrieves the head, while poll removes it.

In GitLab by @swabbur on May 25, 2020, 13:52

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 224

No, element retrieves the head, while remove removes it.

In GitLab by @wubero on May 25, 2020, 13:53

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 192

Ah didn't see that.

In GitLab by @wubero on May 25, 2020, 13:53

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 224

Ah didn't see that.

In GitLab by @JimVliet on May 25, 2020, 13:59

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 20

in between -> inbetween

In GitLab by @JimVliet on May 25, 2020, 13:59

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 20

Besides that, looks good :)

In GitLab by @JimVliet on May 25, 2020, 14:04

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 238

What is the order in which the elements are removed if there are multiple objects that are 'equal' to each other? Specifically whether the order changes after sorting, etc.

In GitLab by @swabbur on May 25, 2020, 14:46

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 20

I thought so as well, Intellij autocorrected it to 'in between'. I'll fix it right away.

In GitLab by @swabbur on May 25, 2020, 14:53

Commented on src/main/java/net/glowstone/util/SortableBlockingQueue.java line 238

I think the sort method of the ArrayList class guarantees that the order of elements that are equal does not change. However, in our application, there is no guarantee on the ordering of inserted elements. Therefore, we cannot provide a guarantee on the order of elements that are equal after sorting either.

In GitLab by @swabbur on May 25, 2020, 14:56

added 1 commit