Implement the BlockingQueue as a wrapper around the QueueFile #201

pschichtel · 2018-10-24T23:39:00Z

The resulting BlockingFileQueue is thread-safe and unbounded.
Thread-safety is implemented with a single lock around all operations against the backing queue.

Closes #198

tape/src/main/java/com/squareup/tape2/BlockingFileQueue.java

…e QueueFile The resulting BlockingFileQueue is thread-safe and unbounded. Thread-safety is implemented with a single lock around all operations against the backing queue.

pschichtel · 2018-10-25T00:57:43Z

I switched to the ObjectQueue as the backing structure as I generally agree that it's more useful, however I'm not entirely sure what overhead this adds in case I'm really only using byte[] buffers (as I am in the project where code this originally comes from).

pschichtel · 2018-10-25T00:59:50Z

The current implementation is also unbounded and I don't intend to implement an upper bound because the backing queue also doesn't have an upper bound.
If an upper bound is added to the QueueFile in the future it would be good to add one here as well.

pschichtel · 2018-10-25T14:22:11Z

I'll take a look into directly accessing QueueFile for BlockingQueue<byte[]> instance without duplicating the whole class.

…e QueueFile Switched to an ObjectQueue<T> as the backing store and delegate closable.

…e QueueFile QueueFile is now an ObjectQueue<byte[]> and can thus be used without overhead in the blocking queue. ObjectQueue's file():QueueFile method has been moved to FileObjectQueue where it fits better as InMemoryObjectQueue doesn't have a backing file. Removed the ByteArrayConverter again as it was the initial attempt to optimize for byte[] queues.

…e QueueFile sneakyfy the IO exception as done in other places.

…e QueueFile Increased coverage and minor cleanup

pschichtel · 2018-10-26T00:16:49Z

@f2prateek So that's the state I would go with. In order to implement ObjectQueue<byte[]> on QueueFile I had to break the current API a little as file():QueueFile on ObjectQueue conflicted with file():File in QueueFile, so I moved file():QueueFile to FileObjectQueue. This way we don't have the noop file():QueueFile on InMemoryObjectQueue and QueueFile can implement ObjectQueue<byte[]> basically as-is.

tape/src/main/java/com/squareup/tape2/QueueFile.java

tape/src/main/java/com/squareup/tape2/BlockingObjectQueue.java

…e QueueFile minor cleanup

NightlyNexus · 2018-10-26T21:29:59Z

tape/src/main/java/com/squareup/tape2/BlockingObjectQueue.java

+ *
+ * @param <E> the element type
+ */
+public class BlockingObjectQueue<E> implements BlockingQueue<E>, Closeable {


NightlyNexus · 2018-10-26T21:31:04Z

tape/src/main/java/com/squareup/tape2/BlockingObjectQueue.java

+
+  private final ObjectQueue<E> queue;
+
+  public BlockingObjectQueue(ObjectQueue<E> queue) {


how about just having the static factory?

NightlyNexus · 2018-10-26T21:31:45Z

tape/src/main/java/com/squareup/tape2/BlockingObjectQueue.java

+   * @param <T> the element type
+   * @return a BlockObjectQueue implementation
+   */
+  public static <T> BlockingObjectQueue<T> create(QueueFile qf, ObjectQueue.Converter<T> conv) {


maybe just take in an ObjectQueue<T>?

These are convenience methods, I'd rather remove the static factories than the constructor. I did them similar to the one in ObjectQueue.

NightlyNexus · 2018-10-26T21:33:27Z

tape/src/main/java/com/squareup/tape2/BlockingObjectQueue.java

+   * @param qf the queue file which should not be shared to other places
+   * @return a BlockObjectQueue implementation
+   */
+  public static BlockingObjectQueue<byte[]> create(QueueFile qf) {


ditto above. maybe can remove this.

NightlyNexus · 2018-10-26T21:35:24Z

tape/src/main/java/com/squareup/tape2/BlockingObjectQueue.java

+      return true;
+    } catch (IOException e) {
+      QueueFile.<Error>getSneakyThrowable(e);
+      return false;


how about throw QueueFile.<Error>getSneakyThrowable(e);? then we don't need the unreachable return statement.

Throwing outside the method doesn't work (I tried it, do you know a way that works?).

hm, should work. there are some existing usages.

tape/tape/src/main/java/com/squareup/tape2/FileObjectQueue.java

Line 97 in 445cd3f

throw QueueFile.<Error>getSneakyThrowable(e);

It must have been late... :D

NightlyNexus · 2018-10-26T22:12:40Z

tape/src/main/java/com/squareup/tape2/BlockingObjectQueue.java

+    try {
+      queue.clear();
+    } catch (IOException e) {
+      QueueFile.<Error>getSneakyThrowable(e);


nit: throws for consistency and clarity.

NightlyNexus · 2018-10-26T22:13:04Z

tape/src/main/java/com/squareup/tape2/ObjectQueue.java

@@ -25,9 +26,6 @@
    return new InMemoryObjectQueue<>();
  }

-  /** The underlying {@link QueueFile} backing this queue, or null if it's only in memory. */
-  public abstract @Nullable QueueFile file();


this has value. let's leave it.
if we do make QueueFile an ObjectQueue, we could rename this API to queueFile().

with file():QueueFile in ObjectQueue you would need to null-check on the result to be save, now with the method in FileObjectQueue you have to check for the implementation and are then guaranteed to get a QueueFile back, not a huge difference.
I think this could be improved by making the factory methods return the specific implementations.

NightlyNexus · 2018-10-26T22:13:39Z

tape/src/main/java/com/squareup/tape2/QueueFile.java

@@ -424,11 +423,6 @@ private long remainingBytes() {
    return fileLength - usedBytes();
  }

-  /** Returns true if this queue contains no entries. */
-  public boolean isEmpty() {


let's leave this.

It's inherited from ObjectQueue

NightlyNexus · 2018-10-26T22:14:04Z

tape/src/main/java/com/squareup/tape2/QueueFile.java

-   *
-   * @throws NoSuchElementException if the queue is empty
-   */
-  public void remove() throws IOException {


and leave this. people use QueueFile directly, and the convenience APIs are convenient!

It's inherited from ObjectQueue

NightlyNexus · 2018-10-26T22:14:49Z

tape/src/main/java/com/squareup/tape2/QueueFile.java

@@ -52,7 +51,7 @@
 *
 * @author Bob Lee ([email protected])
 */
-public final class QueueFile implements Closeable, Iterable<byte[]> {
+public final class QueueFile extends ObjectQueue<byte[]> {


hmmmm, could you get the same functionality by making a FileObjectQueue<byte[]>? a little roundabout, but it keeps QueueFile focused.

I don't see the benefit, can you elaborate?

…e QueueFile more cleanup and a new test

…e QueueFile directly throw the sneaky exceptions

pschichtel · 2018-11-13T01:41:29Z

Any updates here?

pschichtel · 2022-11-06T00:22:54Z

@NightlyNexus I guess this won't be merged?

f2prateek reviewed Oct 24, 2018

View reviewed changes

tape/src/main/java/com/squareup/tape2/BlockingFileQueue.java Outdated Show resolved Hide resolved

Closes square#198: Implement the BlockingQueue as a wrapper around th…

db76821

…e QueueFile The resulting BlockingFileQueue is thread-safe and unbounded. Thread-safety is implemented with a single lock around all operations against the backing queue.

pschichtel force-pushed the blocking-queue branch 4 times, most recently from 7fb0b22 to 27d865b Compare October 25, 2018 00:52

pschichtel force-pushed the blocking-queue branch 2 times, most recently from 75f2593 to 6764bc3 Compare October 25, 2018 22:42

Closes square#198: Implement the BlockingQueue as a wrapper around th…

c799abe

…e QueueFile Switched to an ObjectQueue<T> as the backing store and delegate closable.

pschichtel force-pushed the blocking-queue branch from 6764bc3 to c799abe Compare October 25, 2018 22:53

pschichtel added 3 commits October 26, 2018 01:15

Closes square#198: Implement the BlockingQueue as a wrapper around th…

fce3a3a

…e QueueFile sneakyfy the IO exception as done in other places.

Closes square#198: Implement the BlockingQueue as a wrapper around th…

4f785e1

…e QueueFile Increased coverage and minor cleanup

pschichtel commented Oct 26, 2018

View reviewed changes

tape/src/main/java/com/squareup/tape2/QueueFile.java Outdated Show resolved Hide resolved

pschichtel commented Oct 26, 2018

View reviewed changes

tape/src/main/java/com/squareup/tape2/BlockingObjectQueue.java Outdated Show resolved Hide resolved

pschichtel commented Oct 26, 2018

View reviewed changes

tape/src/main/java/com/squareup/tape2/BlockingObjectQueue.java Show resolved Hide resolved

Closes square#198: Implement the BlockingQueue as a wrapper around th…

4ae94a2

…e QueueFile minor cleanup

NightlyNexus reviewed Nov 3, 2018

View reviewed changes

pschichtel added 2 commits November 3, 2018 02:48

Closes square#198: Implement the BlockingQueue as a wrapper around th…

46c3ddd

…e QueueFile more cleanup and a new test

Closes square#198: Implement the BlockingQueue as a wrapper around th…

f6c2a56

…e QueueFile directly throw the sneaky exceptions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the BlockingQueue as a wrapper around the QueueFile #201

Implement the BlockingQueue as a wrapper around the QueueFile #201

pschichtel commented Oct 24, 2018

pschichtel commented Oct 25, 2018

pschichtel commented Oct 25, 2018 •

edited

Loading

pschichtel commented Oct 25, 2018

pschichtel commented Oct 26, 2018 •

edited

Loading

NightlyNexus Oct 26, 2018

NightlyNexus Oct 26, 2018

NightlyNexus Oct 26, 2018

pschichtel Nov 3, 2018 •

edited

Loading

NightlyNexus Oct 26, 2018

NightlyNexus Oct 26, 2018

pschichtel Nov 3, 2018

NightlyNexus Nov 3, 2018

pschichtel Nov 3, 2018

NightlyNexus Oct 26, 2018

NightlyNexus Oct 26, 2018

pschichtel Nov 3, 2018

NightlyNexus Oct 26, 2018

pschichtel Nov 3, 2018

NightlyNexus Oct 26, 2018

pschichtel Nov 3, 2018

NightlyNexus Oct 26, 2018

pschichtel Nov 3, 2018

pschichtel commented Nov 13, 2018

pschichtel commented Nov 6, 2022


		private final ObjectQueue<E> queue;

		public BlockingObjectQueue(ObjectQueue<E> queue) {

Implement the BlockingQueue as a wrapper around the QueueFile #201

Are you sure you want to change the base?

Implement the BlockingQueue as a wrapper around the QueueFile #201

Conversation

pschichtel commented Oct 24, 2018

pschichtel commented Oct 25, 2018

pschichtel commented Oct 25, 2018 • edited Loading

pschichtel commented Oct 25, 2018

pschichtel commented Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pschichtel Nov 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pschichtel commented Nov 13, 2018

pschichtel commented Nov 6, 2022

pschichtel commented Oct 25, 2018 •

edited

Loading

pschichtel commented Oct 26, 2018 •

edited

Loading

pschichtel Nov 3, 2018 •

edited

Loading