pos-print: add async receipt printing #65

tsbonev · 2018-09-19T12:40:15Z

Added a receipt repository in mongodb and an async printing service, utilizing the mongo capped collection and a background thread to print receipts after they have been persisted.

mgenov · 2018-09-19T12:48:45Z

pos-print/src/main/java/com/clouway/pos/print/PosPrintService.java

@@ -39,6 +40,10 @@ public static void main(String[] args) {
      new PersistentModule(client, commandCLI.dbName())
    );

+    BackgroundReceiptPrinter backgroundPrinter = injector.getInstance(BackgroundReceiptPrinter.class);


You can use guava services about this as it will enforce lifecycle (start/stop) of the printing service.

Implemented Guava services for the background thread.

mgenov · 2018-09-19T12:49:29Z

pos-print/src/main/java/com/clouway/pos/print/adapter/db/BackgroundReceiptPrinter.kt

+ */
+interface BackgroundReceiptPrinter {
+  fun run()
+  fun printReceipts(collection: MongoCollection<Document>)


BackgroundReceiptPrinter interface gets coupled to mongodb which doesn't sound good. It would be better db to be passed to the constructor.

I will implement an interface to abstract the receipt queue from the background thread and that should clear up these dependencies.

mgenov · 2018-09-19T12:50:12Z

pos-print/src/main/java/com/clouway/pos/print/adapter/db/BackgroundReceiptPrinterService.kt

+
+  override fun run() {
+    logger.info("Starting background printing service")
+    repository.getCappedCollection().drop()


This drop will cause issues If app is re-started and some receipts are not printed.

All of the receipts are stored in another collection before being passed to the capped one, so none would be lost, the drop is there because a restart would print all of the contained receipts and removing a single receipt from the capped collection isn't allowed.

The use of a capped collection here seemed to mimic a normal Java queue as it would need to be dropped at the start and end of the program. On top of that, the mongo tailable cursor was not behaving properly so I switched the implementation for a plain in memory queue that block when asked for the next element and the functionality has remained the same with the bonus of loosing Mongo's capped collection restrictions.

Using of in-memory queue is not a good idea as it doesn't allow multiple instances of the pos-print service to be used which removes the redundancy.

Take a look into this one:
http://learnmongodbthehardway.com/schema/queues/

A queue in the setting of MongoDB, means a collection where inserted documents are kept in an order, either by the creation data of the document or by a priority assigned to the document. Multiple publishers can be inserting new documents into the queue at the same time, but each document can only be delivered to a single listener.

e.g this is what is expected from search service.

Multiple instances would all have their own in-memory blocking queue, why would there be a need for a shared queue between the instances?

Queue separation per IP my do it actually.

Also with this in memory queue we can implement logic on the app start, that checks for a not printed receipts and then schedule them in the queue, like that if we restart the app it will load the not printed receipts and trey to print them

Using the CappedCollections required the both operations : put in the collection and put in the capped collection to be executed in a transaction(I'm not a hundred 100% sure)

Yes, it's not atomic now. I suppose it would be better single not capped collection to be used instead as till findAndModify may update it in atomic manner.

Other solution could be creating separate queue for each pos-printer.

Instances have to take free printer and use it ... this adds a lot of complexity and this is why it would be better to be in-memory.

As final: the in-memory solution sounds reasonable for know. If we encounter issues then we will may consider implementing another options.

I agree with you. At this time the app works syncronous and there a just a few problems.
I think that we should provide and a rest services, to serve the last recepts and a rest service to schedule already saved receipt for printing again(failed receipts). Like that we could provide simple table with that contains last 50 receipts and show button for rescheduling of the failed.

mgenov · 2018-09-19T12:52:48Z

...rint/src/test/java/com/clouway/pos/print/adapter/http/BackgroundReceiptPrinterServiceTest.kt

+      oneOf(notifier).notifyPrinted(acceptedPrintingResponse)
+    }
+
+    collection.insertOne(receiptDoc)


Looks really strange to be tested like that as the test knows the structure of the receipt in the queue. The test shouldn't know this and should deal with Receipt object or ReceiptScheduledForPrinting.

Separated mongo logic from the background thread's dependencies.

mgenov · 2018-09-19T12:53:44Z

pos-print/src/test/java/com/clouway/pos/print/persistent/PersistentReceiptRepositoryTest.kt

+  fun requeueingQueuedReceiptThrowsException() {
+    repository.queue(anyReceipt, sourceIp, isFiscal)
+
+    Thread.sleep(500)


Sleep is not a good idea in tests.

Removed thread sleeps from tests.

mlesikov · 2018-09-19T13:02:15Z

pos-print/src/main/java/com/clouway/pos/print/adapter/db/BackgroundReceiptPrinterService.kt

+  }
+
+  @Suppress("UNCHECKED_CAST")
+  private fun adaptReceipt(receiptDoc: Document): Receipt {


you should work with the documents only in the persistence adapter.
You can have PrinterQueue interfece

interface PrinterQueue { fun first():Receipt fun add(receipt: Receipt) fun clear() }

and you should work with it here in the logic

Implemented a PrintQueue interface that provides methods to add a receipt, check for a next receipt, get the next receipt and clear itself.

tsbonev · 2018-09-20T09:48:03Z

PR is up to date with requested changes.

mgenov · 2018-09-20T11:13:54Z

pos-print/src/test/java/com/clouway/pos/print/core/BackgroundReceiptPrintWorkerTest.kt

+  var cleaner = DatastoreCleaner(dataStoreRule.db())
+
+
+  private fun Mockery.expecting(block: Expectations.() -> Unit) {


Private function should be last in the file.

Moved private functions to the bottom of the file.

mgenov · 2018-09-20T11:16:10Z

pos-print/src/main/java/com/clouway/pos/print/core/PrintNotifier.kt

+ *
+ * @author Tsvetozar Bonev ([email protected])
+ */
+interface PrintNotifier {


PrintNotifier doesn't sound good as interface. PrintingListener sounds better as name.

onReceiptPrinted(receipt: Receipt, printStatus: PrintStatus)

Changed interface name to PrintingListener and implemented the suggested method signature.

mgenov · 2018-09-20T11:17:47Z

pos-print/src/main/java/com/clouway/pos/print/core/PrintQueue.kt

+   * Checks if there is a next receipt, blocks until a next
+   * one is found.
+   */
+  fun hasNext(): Boolean


I'm not sure that you need hasNext. next() could block.

For example take a look into: https://docs.oracle.com/javase/9/docs/api/java/util/concurrent/ArrayBlockingQueue.html#poll--

Implemented blocking via the ArrayBlockingQueue take() method.

mgenov · 2018-09-20T11:18:42Z

pos-print/src/main/java/com/clouway/pos/print/PosPrintService.java

    HttpBackend backend = new HttpBackend(commandCLI.httpPort(), injector);
    backend.start();

+    ReceiptPrintWorker backgroundPrinter = injector.getInstance(ReceiptPrintWorker.class);


ReceiptPrintingService sounds better then worker.

Renamed to ReceiptPrintingService.

mgenov · 2018-09-20T11:19:24Z

pos-print/src/main/java/com/clouway/pos/print/PosPrintService.java

    HttpBackend backend = new HttpBackend(commandCLI.httpPort(), injector);
    backend.start();

+    ReceiptPrintWorker backgroundPrinter = injector.getInstance(ReceiptPrintWorker.class);
+    backgroundPrinter.start();
+
    System.out.printf("POS Print Service is up and running on port: %d\n", commandCLI.httpPort());

    Runtime.getRuntime().addShutdownHook(new Thread(() -> {


You have to shutdown printing service when termination signal is received.

awaitTerminated() won't return, is it necessary for it to be called or should I just leave it as stopAsync()?

mgenov · 2018-09-20T11:23:52Z

pos-print/src/main/java/com/clouway/pos/print/adapter/http/PrintServiceV2.kt

+      Reply.with(receiptStatus).ok()
+    }catch (ex: ReceiptNotInQueueException){
+      logger.error("Receipt not found")
+      Reply.saying<Int>().notFound()


saying<Int> is strange. You can use Any or Unit I suppose.

Changed to be saying.

mgenov · 2018-09-20T11:25:42Z

pos-print/src/main/java/com/clouway/pos/print/core/BackgroundReceiptPrintWorker.kt

+  override fun printReceipts() {
+    while (queue.hasNext()) {
+      val receiptWithSource = queue.next()
+      println(receiptWithSource)


logger could be used instead of println

Removed forgotten testing printlns from code.

mgenov · 2018-09-20T11:26:37Z

pos-print/src/main/java/com/clouway/pos/print/core/BackgroundReceiptPrintWorker.kt

+        if (printResponse.warnings.contains(Status.FISCAL_RECEIPT_IS_OPEN)
+          || printResponse.warnings.contains(Status.NON_FISCAL_RECEIPT_IS_OPEN)) {
+          logger.info("Receipt printing accepted")
+          repository.finishPrinting(receipt.receiptId)


The repository could be responsible for the notification. This is going to remove notifier/listener dependency from here.

I don't think the repository should be responsible for the notification, as the listener handles a PrintReceiptResponse which the repository otherwise doesn't have to know about.

You are partially right but:

Here you have 4 dependencies:

class BackgroundReceiptPrintWorker @Inject constructor(private var repository: ReceiptRepository, private var factory: PrinterFactory, private var notifier: PrintNotifier, private var queue: PrintQueue)

which causes tests to become bloating with a lot of code due the number of dependencies. This is a smell and need to be considered split of them.

Should I make the repository depend on the listener interface or should I scrap it and write a hard-coded variant?

Could you elaborate "write a hard-coded variant?"

Some snippet will be helpful.

Currently I only have one implementation of the listener that waits for a second and prints to the console which I used for testing.

class ConsolePrintingListener : PrintingListener { override fun notifyPrinted(printResponse: PrintReceiptResponse) { Thread.sleep(1000) println(printResponse) } }

My question is if I should pass the repository this interface as a dependency, or if I should have something like a logger that logs when finishReceipt or failReceipt are called.

Thread.sleep in testing is not a good idea.

Actually both the Repository and ConsolePrintingListener could implement PrintingListener interface. The persistence will record the result whether the console will print it to the console and this class will replace 2 dependencies with one.

Something like this?

class PersistentReceiptRepository @Inject constructor(private val database: Provider<MongoDatabase>) : ReceiptRepository, PrintingListener {

override fun notifyPrinted(printResponse: PrintReceiptResponse) { val logger = LoggerFactory.getLogger(PrintingListener::class.java) logger.info("Receipt was printed") logger.info(printResponse.warnings.toString()) }

Yes, just change the name from notifyPrinted to something like: onReceiptPrinted

as listener is a class which receive events and not direct orders like "notifyPrinted".

Okay, understood.

mgenov · 2018-09-20T11:27:58Z

pos-print/src/main/java/com/clouway/pos/print/core/InMemoryPrintQueue.kt

+ */
+class InMemoryPrintQueue : PrintQueue {
+
+  private val queue = BlockingArrayQueue<PrintReceiptRequest>()


ArrayBlockingQueue is java equivalent which is better to be used as this one is related to jetty.

My mistake, changed to java blocking queue.

mgenov · 2018-09-20T11:31:36Z

pos-print/src/main/java/com/clouway/pos/print/adapter/http/PrintServiceV2.kt

+      queue.queue(PrintReceiptRequest(dto.receipt, dto.sourceIp, dto.fiscal))
+
+      logger.info("Receipt queued for printing")
+      Reply.with(receiptId).ok()


status 202 is better to be returned as it will indicate that background processing will be performed.

https://httpstatuses.com/202

Changed to return status 202.

mgenov · 2018-09-20T11:33:17Z

pos-print/src/main/java/com/clouway/pos/print/core/ReceiptPrintWorker.kt

+ *
+ * @author Tsvetozar Bonev ([email protected])
+ */
+interface ReceiptPrintWorker {


Not sure that you need this interface at all.

Agreed, but if I remove this interface from the background thread and leave it as a plain AbstractExecutionThreadSerivce, how would I go about binding it's injections with Guice?

Do you need such binding at all?

btw, you can bind and classes too.

I do not, my mistake, will remove the interface.

tsbonev · 2018-09-21T06:53:06Z

Updated Swagger with new v2 api endpoints

tsbonev · 2018-09-28T11:13:44Z

Added #66 to clarify changes to persistence in the PR.
Implementation is WIP.

tsbonev · 2018-10-12T13:23:32Z

Added endpoint to retrieve the last receipts along with their request id and printing status by their creation date and with a limit argument.
Added endpoint to requeue a receipt by request id.

mlesikov · 2018-10-12T13:56:46Z

pos-print/src/main/java/com/clouway/pos/print/adapter/http/PrintServiceV2.kt

+      receiptList.add(ReceiptWithStatusDTO(it.value, it.key, Gson().toJson(receipts[it.key]!!)))
+    }
+
+    return Reply.with(receiptList).ok()


Reply.with(receiptList).as(GsonTransport::class.java).ok()

mlesikov · 2018-10-12T13:59:55Z

pos-print/src/main/java/com/clouway/pos/print/adapter/http/PrintServiceV2.kt

+    }
+  }
+
+  internal data class ReceiptWithStatusDTO(val status: String, val requestId: String, val receiptJson: String)


suggest

ReceiptDTO(val id:String, val amount:String, val requestId: String, val status: String, val isFiscal:Boolean, val amount:Double, val referent...)

mlesikov · 2018-10-12T14:02:07Z

pos-print/src/main/java/com/clouway/pos/print/adapter/http/PrintServiceV2.kt

+  }
+
+  internal data class ReceiptWithStatusDTO(val status: String, val requestId: String, val receiptJson: String)
+  internal data class ReceiptDTO(val sourceIp: String = "", val operatorId: String = "", val fiscal: Boolean = false, val receipt: Receipt = Receipt.newReceipt().build())


tI think that it is better if this dto is PrintReceiptRequest(...)

it look like you are using the domain object as a dto :
Receipt = Receipt.newReceipt().build()

you should create separate dto object that is used only for parsing the request , then you will have to make transformation from dto to the domain

I implemented a ReceiptWithStatus object that is now stored in persistence, containing information about it's printing status, fiscal status, request id, source ip and its receipt object.

I don't think using the receipt domain object directly here will cause a problem, as the request contains a fully-fledged receipt object within itself already.

StanimirIliev · 2018-10-19T13:40:19Z

pos-print/src/main/java/com/clouway/pos/print/adapter/db/PersistentModule.kt

@@ -1,5 +1,7 @@
 package com.clouway.pos.print.adapter.db

+import com.clouway.pos.print.core.PrintingListener


Unused import

Removed the unused import.

StanimirIliev · 2018-10-19T13:47:26Z

pos-print/src/main/java/com/clouway/pos/print/adapter/db/PersistentReceiptRepository.kt

+    if (receipts()
+        .find(and(
+          eq("receipt.receiptId", receipt.receiptId),
+          eq("isFiscal", receiptRequest.isFiscal))).firstOrNull() != null)


Instead of .firstOrNull() != null you could use .any()

Switched over to using any().

StanimirIliev · 2018-10-19T13:56:20Z

pos-print/src/main/java/com/clouway/pos/print/adapter/db/PersistentReceiptRepository.kt

+          eq("isFiscal", receiptRequest.isFiscal))).firstOrNull() != null)
+      throw ReceiptAlreadyRegisteredException()
+
+    val requestId = UUID.randomUUID().toString()


It is better for me to use an interface for generating ids, this way the testing would be easier and also if you decide to not using UUID you will make changes only in one place

Created an IdGenerator intreface and injected a SimpleUUIDGenerator to the repository.

StanimirIliev · 2018-10-23T06:21:25Z

pos-print/src/main/java/com/clouway/pos/print/core/ReceiptPrintingListener.kt

+ */
+class ReceiptPrintingListener : PrintingListener {
+  override fun onPrinted(receipt: Receipt, printStatus: PrintStatus) {
+    val logger = LoggerFactory.getLogger(PrintingListener::class.java)


It would be better if you bring the initialization out of the function

Must've overlooked this, moved it into the class.

StanimirIliev · 2018-10-23T06:30:10Z

pos-print/src/test/java/com/clouway/pos/print/core/BackgroundReceiptPrintingServiceTest.kt

+  companion object {
+    @ClassRule
+    @JvmField
+    val dataStoreRule = DatastoreRule()


If you use @Rule instead of @ClassRule there is no need to make dataStoreRule static

Making the annotation @rule throws a null pointer and the only way I've managed to get this rule to work is by making it a static ClassRule.

tsbonev added the needs-review label Sep 19, 2018

tsbonev requested a review from mlesikov September 19, 2018 12:43

mgenov requested changes Sep 19, 2018

View reviewed changes

clouway-dev-bot added reviewed and removed needs-review labels Sep 19, 2018

mlesikov reviewed Sep 19, 2018

View reviewed changes

tsbonev force-pushed the async_print_service branch 7 times, most recently from f4b2d07 to 8266969 Compare September 20, 2018 09:08

tsbonev force-pushed the async_print_service branch from 8266969 to eb9fac6 Compare September 20, 2018 10:36

mgenov reviewed Sep 20, 2018

View reviewed changes

tsbonev force-pushed the async_print_service branch 3 times, most recently from a3e83f8 to 1aa75fe Compare September 21, 2018 06:50

tsbonev force-pushed the async_print_service branch 10 times, most recently from c9cebd5 to c6386da Compare October 3, 2018 10:09

tsbonev force-pushed the async_print_service branch 4 times, most recently from b4ff0e5 to 808872f Compare October 12, 2018 13:23

mlesikov reviewed Oct 12, 2018

View reviewed changes

tsbonev force-pushed the async_print_service branch 6 times, most recently from 3a0a1f7 to 8dc026c Compare October 15, 2018 12:59

StanimirIliev suggested changes Oct 23, 2018

View reviewed changes

tsbonev force-pushed the async_print_service branch from 8dc026c to e171f1d Compare October 23, 2018 11:27

pos-print: add async receipt printing

b74a569

tsbonev force-pushed the async_print_service branch from e171f1d to b74a569 Compare October 23, 2018 11:42

StanimirIliev approved these changes Oct 23, 2018

View reviewed changes

		var cleaner = DatastoreCleaner(dataStoreRule.db())


		private fun Mockery.expecting(block: Expectations.() -> Unit) {

		@@ -1,5 +1,7 @@
		package com.clouway.pos.print.adapter.db

		import com.clouway.pos.print.core.PrintingListener

pos-print: add async receipt printing #65

Are you sure you want to change the base?

pos-print: add async receipt printing #65

Conversation

tsbonev commented Sep 19, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlesikov Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsbonev commented Sep 20, 2018

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

mgenov Sep 20, 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

tsbonev Sep 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsbonev Sep 20, 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

tsbonev commented Sep 21, 2018

tsbonev commented Sep 28, 2018

tsbonev commented Oct 12, 2018

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

mlesikov Sep 19, 2018 •

edited

Loading

mgenov Sep 20, 2018 •

edited

Loading

tsbonev Sep 20, 2018 •

edited

Loading

tsbonev Sep 20, 2018 •

edited

Loading