Added retry mechanism parameter for LongWriteOperation #357

martiwi · 2018-01-18T18:16:57Z

PR for issue #352

dariuszseweryn

Hey! Great that you have took on this one!
I have some comments I have put in the code which I think should get addressed. I would also like @uKL to look on this.
Anyway, this will be a very neat feature and definitely very useful!

dariuszseweryn · 2018-01-18T19:38:49Z

rxandroidble/src/main/java/com/polidea/rxandroidble/RxBleConnection.java

@@ -442,6 +450,9 @@ public String toString() {
     */
    Observable<byte[]> writeDescriptor(@NonNull BluetoothGattDescriptor descriptor, @NonNull byte[] data);

+    interface WriteOperationRetryStrategy extends Func2<Integer, Throwable, Boolean> {


This should be placed near WriteOperationAckStrategy

dariuszseweryn · 2018-01-18T19:44:04Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

                .repeatWhen(bufferIsNotEmptyAndOperationHasBeenAcknowledgedAndNotUnsubscribed(
                        writeOperationAckStrategy, byteBuffer, emitterWrapper
                ))
+                .retry(retryStrategy(byteBuffer, batchSize))


I think that we should go with the .retryWhen() function rather than with just .retry(). This is because the .retryWhen() is more flexible. It allows for differentiation of the retry behaviour i.e. one may want to retry the write after some time or depending on other external factor

Indeed, that makes sense.

dariuszseweryn · 2018-01-18T20:09:59Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

        this.bytesToWrite = bytesToWrite;
+        this.retryCounter = 0;


I am not a big fan of variables that are changed as a side effect. This usually makes room for a mistake and makes the code harder to trace.

You mean we should not have a separated counter in this Operation class at all?

I believe that Darek meant putting a reset in a doOnNext. AFAIK you can safely reset the counter before the rx subscription (around line 86.)

uKL · 2018-01-19T07:23:35Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

        this.bytesToWrite = bytesToWrite;
+        this.retryCounter = 0;


I believe that Darek meant putting a reset in a doOnNext. AFAIK you can safely reset the counter before the rx subscription (around line 86.)

uKL · 2018-01-19T07:26:55Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

+            public Boolean call(Integer integer, Throwable throwable) {
+                // Individual counter for each batch payload.
+                retryCounter++;
+                final Boolean retry = writeOperationRetryStrategy.call(retryCounter, throwable);


The biggest question here is to:

Define what data one will need to decide on a retry

Smart way to extend that data without breaking the API in the future (DTO? @dariuszseweryn)

Yup, probably we should go for WriteOperationRetryStrategy with something like:

class LongWriteFailure { final int batchNumber; // final int retryCount; –> this should not be needed as it is trivial for the user to manage final BleException cause; }

You're reading in my mind :)

I came up with the following retryWhen() function:

private static Func1<Observable<? extends Throwable>, Observable<?>> retryOperationStrategy( final WriteOperationRetryStrategy writeOperationRetryStrategy, final ByteBuffer byteBuffer, final int batchSize) { return new Func1<Observable<? extends Throwable>, Observable<?>>() { @Override public Observable<?> call(Observable<? extends Throwable> observable) { return observable .flatMap(applyRetryStrategy()) .map(replaceByteBufferPositionForRetry()); } @NonNull private Func1<Throwable, Observable<WriteOperationRetryStrategy.LongWriteFailure>> applyRetryStrategy() { return new Func1<Throwable, Observable<WriteOperationRetryStrategy.LongWriteFailure>>() { @Override public Observable<WriteOperationRetryStrategy.LongWriteFailure> call(Throwable cause) { if (!(cause instanceof BleException)) { return Observable.error(cause); } final int failedBatchNumber = (byteBuffer.position() - batchSize) / batchSize; final WriteOperationRetryStrategy.LongWriteFailure longWriteFailure = new WriteOperationRetryStrategy.LongWriteFailure(failedBatchNumber, (BleException) cause); return writeOperationRetryStrategy.call(Observable.just(longWriteFailure)); } }; } @NonNull private Func1<WriteOperationRetryStrategy.LongWriteFailure, Object> replaceByteBufferPositionForRetry() { return new Func1<WriteOperationRetryStrategy.LongWriteFailure, Object>() { @Override public Object call(WriteOperationRetryStrategy.LongWriteFailure longWriteFailure) { final int newBufferPosition = longWriteFailure.getBatchNumber() * batchSize; byteBuffer.position(newBufferPosition); return longWriteFailure; } }; } }; }

If you could push another commit to this PR I could then comment on specific lines

Sure, done!

dariuszseweryn

Better! :)
I have written some comments in the code
We are getting closer

dariuszseweryn · 2018-01-22T15:53:38Z

rxandroidble/src/main/java/com/polidea/rxandroidble/RxBleConnection.java

+        class LongWriteFailure {
+
+            final int batchNumber;
+            final BleException cause;


This actually should be a BleGattCharacteristicException as disabling the BluetoothAdapter is not really retriable...

Both should be final.

Ok for BleGattCharacteristicException but they are already both final :)

Yup. I must be tired...

dariuszseweryn · 2018-01-22T16:02:33Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

+                        final WriteOperationRetryStrategy.LongWriteFailure longWriteFailure =
+                                new WriteOperationRetryStrategy.LongWriteFailure(failedBatchNumber, (BleException) cause);
+
+                        return writeOperationRetryStrategy.call(Observable.just(longWriteFailure));


Not an ideal solution since the writeOperationRetryStrategy will get called each time the a new failure will happen where is could be called only once per subscription. Checkout how the Observable.compose() function or bufferIsNotEmptyAndOperationHasBeenAcknowledgedAndNotUnsubscribed works.

Basically this:

final int failedBatchNumber = (byteBuffer.position() - batchSize) / batchSize; final WriteOperationRetryStrategy.LongWriteFailure longWriteFailure = new WriteOperationRetryStrategy.LongWriteFailure(failedBatchNumber, (BleException) cause);

could be separated to a Observable.map() and the writeOperationRetryStrategy could be treated as a Observable.Transformer (see .compose())

How close am I with something like that :) :

@Override public Observable<?> call(Observable<? extends Throwable> emittedOnWriteFailure) { return writeOperationRetryStrategy.call( emittedOnWriteFailure.map(mapToLongWriteFailureObject()) ) .doOnNext(replaceByteBufferPositionForRetry()); }

No other Throwable than BleException can be emitted?

No other than BleGattCharacteristicException can be retried... Again, new commit in this PR makes it easier to see the whole picture. :)

Sorry, I've pushed :)

Yup, I have seen—just have a bit of work now. Will get back to you as soon as possible. 👍

I have checked out your branch and I hope I will have a moment Tomorrow to play with it and see if I can make it a little bit nicer. I cannot think of how to explain this so I will try to show it ;)

dariuszseweryn · 2018-01-22T16:09:57Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

+            }
+
+            @NonNull
+            private Func1<WriteOperationRetryStrategy.LongWriteFailure, Object> replaceByteBufferPositionForRetry() {


This could be a simple .onNext() as it only passes the longWriteFailure

…n#352)

dariuszseweryn · 2018-01-24T21:45:03Z

I didn't find a moment Today to play with it. Tomorrow should be the day

dariuszseweryn

I have looked a bit and made a one small change. I have added more thoughts in this review. This work has potential to be really useful 👍

dariuszseweryn · 2018-01-25T17:37:19Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

+                return new Func1<Throwable, WriteOperationRetryStrategy.LongWriteFailure>() {
+                    @Override
+                    public WriteOperationRetryStrategy.LongWriteFailure call(Throwable throwable) {
+                        final int failedBatchNumber = (byteBuffer.position() - batchSize) / batchSize;


This calculation will be wrong if the last batch will not be equal size to default batchSize. Ideally we should be passing the batch size to this function from the writeBatchAndObserve().

Indeed, but what advantage compared to something like that instead:

if (byteBuffer.hasRemaining()) { return (byteBuffer.position() - batchSize) / batchSize; } else { return byteBuffer.position() / batchSize; }

Ideally we should emit the batches to write byte[] which should be indexed. Those pairs of bytes and indexes should then be written (and retried) accordingly to the strategy to be in line with in as immutable approach as possible. I am not sure how would the performance turn up...

dariuszseweryn · 2018-01-25T17:38:05Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

+            }
+
+            @NonNull
+            private Action1<WriteOperationRetryStrategy.LongWriteFailure> replaceByteBufferPositionForRetry() {


Maybe repositionByteBufferForRetry()?

dariuszseweryn · 2018-01-25T17:39:40Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

+            }
+
+            @NonNull
+            private Func1<Throwable, Observable<Throwable>> canRetryError() {


Maybe toLongWriteFailureOrError()? It would then look like .flatMap(toLongWriteFailureOrError())

You mean merge those two?

.flatMap(canRetryError()) .map(toLongWriteFailureObject())

Yes, as the the .map() contains a pretty straightforward logic. Maybe toLongWriteFailureIfRetryable() (not sure about the spelling)

I've split them based on comment: #357 (comment) :)

It was exactly more about using .compose(writeOperationRetryStrategy)

dariuszseweryn · 2018-01-25T17:41:28Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

+                return emittedOnWriteFailure
+                        .flatMap(canRetryError())
+                        .map(toLongWriteFailureObject())
+                        .compose(writeOperationRetryStrategy)


This is what I have changed which makes it easier to read in my opinion. I will refactor the ACK strategy one day...

dariuszseweryn · 2018-01-25T17:47:23Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

@@ -209,4 +214,58 @@ public Boolean call(Object emission) {
            }
        };
    }
+
+    private static Func1<Observable<? extends Throwable>, Observable<?>> retryOperationStrategy(


Maybe errorIsRetriableAndAccordingTo(...)? Then we could have .retryWhen(errorIsRetriableAndAccordingTo(writeOperationRetryStrategy,...))

…eryn#352)

martiwi · 2018-02-07T07:29:37Z

I have pushed my latest modifications accordingly to your remarks.

dariuszseweryn

Almost there!
I think that we should have tests for this feature
This looks well :)

dariuszseweryn · 2018-02-07T11:06:43Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

+
+            private int calculateFailedBatchNumber(ByteBuffer byteBuffer, int batchSize) {
+                if (byteBuffer.hasRemaining()) {
+                    return (byteBuffer.position() - batchSize) / batchSize;


(byteBuffer.position() / batchSize) - 1 seems slightly easier to understand for me :)

dariuszseweryn · 2018-02-07T11:10:53Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

+                return emittedOnWriteFailure
+                        .flatMap(toLongWriteFailureOrError())
+                        .compose(writeOperationRetryStrategy)
+                        .doOnNext(repositionByteBufferForRetry());


This line should go to above the .compose() as the user could potentially emit a different LongWriteFailure than they were passed which could flaw the reposition of the buffer.

dariuszseweryn · 2018-02-14T21:17:02Z

Hello @martiwi - any news? Maybe I can help you with something?
I see this feature just lacking some unit tests and it would be production ready for 1.5.0 release

martiwi · 2018-02-14T23:09:36Z

Hi there,
I didn't had enough time available to understand correctly how your unit test are designed and how they are working. I've been able to update the current unit tests to have latest changes of this feature. But to test that retry strategy is properly working, I did not managed to. Any help is welcome to speed up on this. Thanks.

uKL · 2018-02-27T14:40:33Z

Hey @martiwi !

Could you tick "Allow edits from maintainers" in your PR? I not able to contribute to unit tests.

martiwi · 2018-02-27T14:49:43Z

Hey @uKL it was already selected, @dariuszseweryn was able to publish before. I've unselected it, and re-selected it. Is it better now ?

…etry_mechanism

uKL · 2018-02-28T08:34:06Z

Ok, I was doing it wrong. :)

martiwi · 2018-03-03T11:58:57Z

Hi @dariuszseweryn - I've added one unit test but I am not sure I am using it right as when running the test in debug mode, the second batch is failing to write but retryWhen Observable is triggered twice. Meaning Characteristic#setValue() is called with first batch number and not the second. Can you check what am I doing wrong? Thanks.

dariuszseweryn · 2018-03-03T12:18:06Z

Hello @martiwi
You can check if the whole chain has not failed at this point by using testSubscriber.assertNoErrors(). Additionally in the current test all batches after the first failed one will not be successfully completed as the subject is called with .onError()

…n#352)

- Fixed issue that caused the last batch to be written with incorrect data (empty byte array) - Added support for errors that were occurring when operation was started (not from the gatt callback)

…s skipped

uKL · 2018-03-09T13:51:34Z

Great job @martiwi!

I've added some tests and fixed two issues + a documentation update.

Added tests for retries during long write operations
Fixed issue that caused the last batch to be written with incorrect data (empty byte array)
Added support for errors that were occurring when the operation was started (not from the gatt callback)
Fixed documentation incorrectly stating that the particular batch is skipped

dariuszseweryn · 2018-03-09T14:09:16Z

...ovy/com/polidea/rxandroidble/internal/operations/OperationCharacteristicLongWriteTest.groovy

+        given:
+        this.writeOperationRetryStrategy = givenWillRetryWriteOperation()
+        this.writeOperationAckStrategy = new ImmediateSerializedBatchAckStrategy()
+        prepareObjectUnderTest(2, [0x1, 0x1, 0x2, 0x2, 0x3, 0x3] as byte[])


Could you make the last batch to have an uneven number of bytes? I think we can have a wrong ByteBuffer positioning on the last batch if is not full

dariuszseweryn · 2018-03-09T14:54:03Z

...main/java/com/polidea/rxandroidble/internal/operations/CharacteristicLongWriteOperation.java

+            }
+
+            private int calculateFailedBatchNumber(ByteBuffer byteBuffer, int batchSize) {
+                return (int) Math.ceil(byteBuffer.position() / (float) batchSize);


doesn't it return a failed batch number + 1?

Having a [1,2,3,4,5] and 2 bytes per batch
If the first batch fails the byteBuffer.position() would return 2
2/2 == 1 and the failed batch index should be 0

Data size 5, batch size 2, failure at the last batch (3rd)
(int) Math.ceil(5 / (float) 2) => 3

Data size 6, batch size 2, failure at the last batch (3rd)
(int) Math.ceil(6 / (float) 2) => 3

Data size 5, batch size 3, failure at the last batch (2nd)
(int) Math.ceil(5 / (float) 3) => 2

Is this what you wanted to check?

calculateFailedBatchNumber is misleading in this situation, either we should return
(int) Math.ceil(byteBuffer.position() / (float) batchSize) - 1
or name the function differently?

It returns the "number" which I assumed starts from number one. LongWriteFailure also assumes "number". Do you think we should switch to zero-based indexes?

Yup. This is user (developer) facing data
I think we should switch to zero-based indexes and rename the function to calculateFailedBatchIndex and longWriteFailure.getBatchIndex() respectively

uKL · 2018-03-20T13:20:05Z

Dear Contributor,

similar to many open source projects we kindly request to sign our CLA if you'd like to contribute to our project. This license is for your protection as a Contributor as well as the protection of Polidea; it does not change your rights to use your own Contributions for any other purpose.

You can find a link here: https://cla-assistant.io/Polidea/RxAndroidBle

Martin WIRTH added 2 commits January 15, 2018 11:37

Added retry mechanism parameter for LongWriteOperation

1a49205

Checkstyle

e752af4

dariuszseweryn requested changes Jan 18, 2018

View reviewed changes

uKL requested changes Jan 19, 2018

View reviewed changes

Change retry mechanism (dariuszseweryn#352)

d6f113e

dariuszseweryn requested changes Jan 22, 2018

View reviewed changes

Martin WIRTH added 2 commits January 23, 2018 08:57

Call WriteOperationRetryStrategy once per subscription (dariuszsewery…

ce02a02

…n#352)

Checkstyle: remove unused import (dariuszseweryn#352)

f54f4ae

Minor refactoring to use .compose() interface.

563e675

dariuszseweryn requested changes Jan 25, 2018

View reviewed changes

Martin WIRTH added 2 commits February 5, 2018 08:46

Change methods names and fix issue for batch number retry (dariuszsew…

03effa5

…eryn#352)

Remove static import for inner class LongWriteFailure and checkstyle.

8570954

dariuszseweryn requested changes Feb 7, 2018

View reviewed changes

Change when reposition of byte buffer is done (dariuszseweryn#352)

f979bfa

Update long write unit tests with retry operation strategy

d39f6ad

uKL self-assigned this Feb 27, 2018

Merge remote-tracking branch 'origin/master' into feature/issue_352_r…

1026205

…etry_mechanism

Add unit test for long write retry strategy

cdddd29

Martin WIRTH and others added 3 commits March 5, 2018 09:03

Unit test: assert no errors when long write is retried (dariuszsewery…

ef0bfe2

…n#352)

- Added tests for retries during long write operations

cf207e2

- Fixed issue that caused the last batch to be written with incorrect data (empty byte array) - Added support for errors that were occurring when operation was started (not from the gatt callback)

- Fixed documentation incorrectly stating that the particular batch i…

8d3f2fe

…s skipped

uKL approved these changes Mar 9, 2018

View reviewed changes

dariuszseweryn reviewed Mar 9, 2018

View reviewed changes

- Fixed positioning of the buffer for uneven data lengths.

b9c0ba2

dariuszseweryn reviewed Mar 9, 2018

View reviewed changes

uKL added 2 commits March 9, 2018 16:16

Changed indexing scheme for long write retry notifications.

db06717

Fixed buffer repositioning, based on a zero based counter.

6dd6dd2

uKL added the enhancement label Mar 12, 2018

uKL added this to the 1.5.0 milestone Mar 12, 2018

dariuszseweryn approved these changes Mar 12, 2018

View reviewed changes

Added documentation for the retry strategy.

0635c28

uKL merged commit c3d2596 into dariuszseweryn:master Mar 12, 2018

dariuszseweryn mentioned this pull request Mar 12, 2018

Retry mechanism on some BLE stack errors #352

Closed

Added retry mechanism parameter for LongWriteOperation #357

Added retry mechanism parameter for LongWriteOperation #357

Conversation

martiwi commented Jan 18, 2018

dariuszseweryn left a 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

dariuszseweryn left a 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

dariuszseweryn commented Jan 24, 2018

dariuszseweryn left a 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

martiwi Jan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martiwi commented Feb 7, 2018

dariuszseweryn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariuszseweryn commented Feb 14, 2018

martiwi commented Feb 14, 2018

uKL commented Feb 27, 2018

martiwi commented Feb 27, 2018

uKL commented Feb 28, 2018

martiwi commented Mar 3, 2018

dariuszseweryn commented Mar 3, 2018

uKL commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uKL Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariuszseweryn Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

uKL commented Mar 20, 2018

martiwi Jan 26, 2018 •

edited

Loading

uKL Mar 9, 2018 •

edited

Loading

dariuszseweryn Mar 9, 2018 •

edited

Loading