Implementing Retry logic [Reliable Channel] #556

debugthings · 2018-02-11T21:55:49Z

Added retry logic that is similar to the .NET framework.

Removes logic from NetworkTransmissionOutput
Uses observers to handle error conditions
Added failed transmission retry
Added partial transmission retry
Aligned response codes to match .NET
Uses existing ExponentialBackOffTimesPolicy

NOTE This clobbers the change in #385 referenced here

https://github.com/Microsoft/ApplicationInsights-Java/blob/7a5e572b2ffd3256898c6bd05dc3bec6754e3629/core/src/main/java/com/microsoft/applicationinsights/internal/channel/common/TransmissionNetworkOutput.java#L172

All exceptions are now handled by ErrorHandler in this line.

https://github.com/Microsoft/ApplicationInsights-Java/blob/094383a25b0f98fe6aa2dd6dd58bae10259492e6/core/src/main/java/com/microsoft/applicationinsights/channel/concrete/inprocess/ErrorHandler.java#L39-L42

For significant contributions please make sure you have completed the following items:

Design discussion issue #
Changes in public surface reviewed
CHANGELOG.md updated

… sender channel.

…ity. Added extension to return the timeout values as expected before.

dhaval24

One general change that is required is Java doc comments are missing all over. We need that especially for new public methods and also some private methods which have important logic.

Try to reduce the scope of public methods exposed if possible. I think some can be done.

I did an initial glance over on this, and we would do a functional review soon.

dhaval24 · 2018-02-11T22:10:43Z

core/build.gradle

@@ -65,6 +66,7 @@ shadowJar {
    relocate 'org.apache.commons', 'com.microsoft.applicationinsights.core.dependencies.apachecommons'
    relocate 'com.google.common', 'com.microsoft.applicationinsights.core.dependencies.googlecommon'
    relocate 'javax.annotation', 'com.microsoft.applicationinsights.core.dependencies.javaxannotation'
+    relocate 'com.google.gson', 'com.microsoft.applicationinsights.core.dependencies.gson'


Do you use gson for actual code? Or this is only for test? If it is only test I would say do not relocate. Relocation usually should be done when it is absolutely necessary.

I added Gson to take care of the parsing of the BackendResponse object that is returned from a partial success. There were no JSON deserializers already included in the solution (at least none that were obvious).

@debugthings makes sense. Gson was used but only for test cases. Since this is now used for production code we do need to shade it. May be we might not need to shade it still. Gradle will pull this as transitive dependency when building. Shading (i.e repacking) to microsoft domain is only needed when it interferes our agent (one example is apache http client). We shade it to avoid instrumenting the calls sdk makes using it. Gson is a widely used library across java and I would rather avoid shading to prevent us from falling into the traps of class path hells.

Fair enough, I also thought that we include it to avoid having to distribute gson with it for instances where this is a drop in addition like a 3rd party.

Yes, that was one reason but, this creates problems of dependency hells if we miss proper shading, and more over if the user uses dependency management, which most do then transient dependencies get pulled down. Further as the SDK grows and we start using more libraries, it would make sense to just shade the one which interfer the operation and leave the rest

dhaval24 · 2018-02-11T22:11:17Z

.../main/java/com/microsoft/applicationinsights/channel/concrete/inprocess/BackendResponse.java

+
+public class BackendResponse {
+
+	public int itemsReceived;


Adjust the additional space

dhaval24 · 2018-02-11T22:13:32Z

...src/main/java/com/microsoft/applicationinsights/channel/concrete/inprocess/ErrorHandler.java

+		validateTransmissionAndSend(args);
+	}
+
+	public boolean validateTransmissionAndSend(TransmissionHandlerArgs args) {


Does this method need to be public? If it is not accessed from outside, reduce the scope to private or default.

The answer is no, I did this to get my tests working in. I attempted to use Mockito for this, but in version 1.10 they do not support private methods or final classes. This makes it hard to mock a number of items.

Since we have the need to document all public methods I can modify the tests to use Reflection to grab the private member.

Ah so mockito version 1.10 does not allow mocking private methods. Is this method a part of interface? That's why I was suggesting since this is used widely we can plug it into the interface and then mock it. If that route is not valid for some reason then we might better use reflection. Reducing the publicly exposed methods will improve our abilities to modify it without affecting anyone. Users can take dependency on public methods and modification of signature can thus be very cumbersome.

Fixed this by using the package scope. Only classes defined in the packages can see the method.

dhaval24 · 2018-02-11T22:15:42Z

...java/com/microsoft/applicationinsights/channel/concrete/inprocess/PartialSuccessHandler.java

+			Gson gson = gsonBuilder.create();
+			backend = gson.fromJson(response, BackendResponse.class);
+		} catch (Throwable t) {
+			InternalLogger.INSTANCE.trace("Error deserializing backend response with Gson, message: %s",


Please print the stack trace also. ExceptionUtils can be used for this.

dhaval24 · 2018-02-11T22:16:44Z

...java/com/microsoft/applicationinsights/channel/concrete/inprocess/PartialSuccessHandler.java

+
+				// In case the 206 was false we can break here
+				if(beR != null && (beR.itemsAccepted == beR.itemsReceived))
+				{


I think there should be a log message here, with lowest severity.

dhaval24 · 2018-02-11T22:18:42Z

...java/com/microsoft/applicationinsights/channel/concrete/inprocess/PartialSuccessHandler.java

+					case HttpStatus.SC_REQUEST_TIMEOUT:
+					case HttpStatus.SC_INTERNAL_SERVER_ERROR:
+					case HttpStatus.SC_SERVICE_UNAVAILABLE:
+					case 429:


So we don't handle 429 ?

Eh? In the partial success handler any an all error messages are treated the same. In this instance the partial success will try to resend any of these items in the next transmission.

https://github.com/Microsoft/ApplicationInsights-dotnet/blob/0fe5edf099ecdad37990be0c776b300e7fbcff4a/src/ServerTelemetryChannel/Implementation/PartialSuccessTransmissionPolicy.cs#L107-L111

Ah got it. So this means if partial message was accepted, then we would retry the whole packet correct?

dhaval24 · 2018-02-11T22:20:33Z

...java/com/microsoft/applicationinsights/channel/concrete/inprocess/PartialSuccessHandler.java

+				}
+			} catch (IOException e1) {
+				// TODO Auto-generated catch block
+				e1.printStackTrace();


Don't print stack trace. Instead dump it using Internal Logger

dhaval24 · 2018-02-11T22:24:47Z

...ain/java/com/microsoft/applicationinsights/channel/concrete/inprocess/ThrottlingHandler.java

+
+	}
+
+	private static Date convertToDateToGmt(Date date) {


We need java docs for all the methods and their description. It is crucial for all public methods and private methodsl like this where there is core logic.

dhaval24 · 2018-02-11T22:25:57Z

...ain/java/com/microsoft/applicationinsights/channel/concrete/inprocess/ThrottlingHandler.java

+		validateTransmissionAndSend(args);
+	}
+
+	public boolean validateTransmissionAndSend(TransmissionHandlerArgs args) {


Similar to previous does this method need to be public. More-ever can we create an abstraction for this too, like an interface because it seems like this is being used in different handler classes you created.

Fixed this by using the package scope. Only classes defined in the packages can see the method.

dhaval24 · 2018-02-11T22:32:47Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionNetworkOutput.java

+				return true;
+
+			} catch (ConnectionPoolTimeoutException e) {
+				ex = e;


All this places we need to dump the stack trace. Please use ExceptionUtils to do so and pass it in logger.

dhaval24 · 2018-02-12T16:28:06Z

...rc/main/java/com/microsoft/applicationinsights/internal/channel/TransmissionHandlerArgs.java

+	public void setResponseCode(int code) { this.responseCode = code;}
+	public int getResponseCode() { return this.responseCode;}
+
+	private Throwable exception;


@debugthings do we ever get a Throwable here? I think most of the time it would get an instance of Exception class. If you know any place where this could be Throwable please let me know

From the original implementation they catch Throwable for logging. I also consume it here incase there is an Error and want to retry the transmission.

https://github.com/Microsoft/ApplicationInsights-Java/blob/1e5d72cadd24cb75b46d577ef0f8c3828fc87e19/core/src/main/java/com/microsoft/applicationinsights/internal/channel/common/TransmissionNetworkOutput.java#L190-L193

Which is then added here
https://github.com/Microsoft/ApplicationInsights-Java/blob/1e5d72cadd24cb75b46d577ef0f8c3828fc87e19/core/src/main/java/com/microsoft/applicationinsights/internal/channel/common/TransmissionNetworkOutput.java#L209

And then null checked here
https://github.com/Microsoft/ApplicationInsights-Java/blob/1e5d72cadd24cb75b46d577ef0f8c3828fc87e19/core/src/main/java/com/microsoft/applicationinsights/channel/concrete/inprocess/ErrorHandler.java#L56

dhaval24 · 2018-02-12T16:55:02Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionPolicyManager.java

@@ -46,8 +50,16 @@
 *
 * Created by gupele on 6/29/2015.
 */
-public final class TransmissionPolicyManager implements Stoppable {
-
+public final class TransmissionPolicyManager implements Stoppable, TransmissionHandlerObserver {


So we had to change the public class also, I think which is fine. However, if some of our users if they had taken dependency on this. we would break them by this change. I am not sure we might need to consider a separate release for this, mentioning that there can be some breaking changes.

Maybe. We added four public methods to this but did not remove any; and didn't change the implementation of existing methods. That shouldn't break the contract and the code should execute as expected. Might well warrant a deeper discussion

dhaval24 · 2018-02-12T16:59:48Z

...java/com/microsoft/applicationinsights/channel/concrete/inprocess/PartialSuccessHandler.java

+	 * @return A List<> of each sent item
+	 */
+	List<String> generateOriginalItems(TransmissionHandlerArgs args) {
+		List<String> originalItems = new ArrayList<String>();


Types in <> in Java 7 and onwards can be auto-inferred and is more efficient. This can be replaced with
List originalItems = new ArrayList<>();

dhaval24 · 2018-02-12T17:02:50Z

...java/com/microsoft/applicationinsights/channel/concrete/inprocess/PartialSuccessHandler.java

+				List<String> originalItems = generateOriginalItems(args);
+
+				// Somehow the amount of items received and the items sent do not match
+				if (beR != null && (originalItems.size() != beR.itemsReceived)) {


I would suggest to make this condition more robust. This could be more apt
if (beR != null && (originalItems.size() < beR.itemsReceived))

Not sure I agree, if these numbers don't match then there was a problem with the transmission and we're in an invalid state I would think. Either the original content was broken or the response is broken

dhaval24 · 2018-02-12T17:04:07Z

...java/com/microsoft/applicationinsights/channel/concrete/inprocess/PartialSuccessHandler.java

+					return false;
+				}
+
+				if (beR != null && (beR.itemsAccepted != beR.itemsReceived)) {


Similarly for this the condition can be changed to
if (beR != null && (beR.itemsAccepted < beR.itemsReceived)
This makes it logically more clear and better readable code. IMO the condition in itself is also more robust.

dhaval24 · 2018-02-12T17:09:21Z

...java/com/microsoft/applicationinsights/channel/concrete/inprocess/PartialSuccessHandler.java

+				if (beR != null && (beR.itemsAccepted != beR.itemsReceived)) {
+
+					List<String> newTransmission = new ArrayList<String>();
+					for (BackendResponse.Error e : beR.errors) {


Why don't we replace this for-each loop with a for loop like this :
for (int i = 0; i < ber.errors.length; ++i). This would help simplify the index operation being used mutiple times. We can the just do
if (i < originalItems.size()) {
newTransmission.add(originalItems.get(i));
}

The index of the items in the errors array is not guaranteed to be the index of the original item sent.

errors[0] = Error() { index = 2, responseCode = 500, message = "Internal Error" }

Ah this is a catch. Got it

dhaval24 · 2018-02-12T17:21:17Z

...ain/java/com/microsoft/applicationinsights/channel/concrete/inprocess/ThrottlingHandler.java

+	boolean validateTransmissionAndSend(TransmissionHandlerArgs args) {
+		if (args.getRetryHeader() != null && args.getTransmission() != null
+				&& args.getTransmissionDispatcher() != null) {
+			args.getTransmission().incrementNumberOfSends();


I see that PartialResponseHandler class doesn't increment the number of sends like this. Is there a reason why it is omitted there? Because eventually we are retrying correct?

In this instance we're creating a brand new transmission to send based on the result of the old one. We are not going to dispatch the entire transmission so no need to increment.

dhaval24 · 2018-02-12T17:35:16Z

...ain/java/com/microsoft/applicationinsights/channel/concrete/inprocess/ThrottlingHandler.java

+			Date date = formatter.parse(retryAfterAsString);
+
+			Date now = Calendar.getInstance().getTime();
+			long retryAfterAsSeconds = (date.getTime() - convertToDateToGmt(now).getTime()) / 1000;


Seems like this calculation is always fixed. I might have missed something , please point out to where we are doing exponential backoff in this case.

For Throttling requests the server provides the backoff time in the form of the Retry-After header. If that header is missing or invalid (will throw) I call backoff in the catch block

Makes sense. I would say to mention a comment!

dhaval24 · 2018-02-12T17:41:50Z

.../com/microsoft/applicationinsights/internal/channel/common/SenderThreadLocalBackOffData.java

@@ -68,6 +68,15 @@ public SenderThreadLocalBackOffData(long[] backOffTimeoutsInMillis, long addMill
    public boolean isTryingToSend() {
        return currentBackOffIndex != -1;
    }
+


Missing Java docs for getCurrentBackOffMillis()

Not needed, removed

dhaval24 · 2018-02-12T17:50:45Z

.../com/microsoft/applicationinsights/internal/channel/common/SenderThreadLocalBackOffData.java

+       }
+   }
+
+    public long backOffTimerValue() {


Java docs missing for this method

dhaval24 · 2018-02-12T17:52:18Z

...a/com/microsoft/applicationinsights/internal/channel/common/SenderThreadsBackOffManager.java

@@ -75,6 +80,11 @@ public synchronized void stopAllSendersBackOffActivities() {
        }
        stopped = true;
    }
+
+    public long getCurrentBackoffMillis() {


dhaval24 · 2018-02-12T18:06:10Z

.../main/java/com/microsoft/applicationinsights/internal/channel/common/TransmissionPolicy.java

@@ -24,8 +24,9 @@
 /**
 * Created by gupele on 6/29/2015.
 */
-enum TransmissionPolicy {
+public enum TransmissionPolicy {


Update java docs for this

Removed public.

dhaval24 · 2018-02-12T18:07:21Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionNetworkOutput.java

 import java.util.concurrent.TimeUnit;

 /**
- * The class is responsible for the actual sending of {@link com.microsoft.applicationinsights.internal.channel.common.Transmission}
+ * The class is responsible for the actual sending of
+ * {@link com.microsoft.applicationinsights.internal.channel.common.Transmission}
 *
 * The class uses Apache's HttpClient framework for that.
 *
 * Created by gupele on 12/18/2014.
 */
 public final class TransmissionNetworkOutput implements TransmissionOutput {


There are bunch of methods in this class which do not have java docs. It would be good if we can update those places as we are changing this part.

Added docs for public and private methods.

dhaval24 · 2018-02-12T18:09:46Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionPolicyManager.java

-
+public final class TransmissionPolicyManager implements Stoppable, TransmissionHandlerObserver {
+
+	private final int DEFAULT_MAX_SECONDS_TO_PAUSE_AFTER_MAX_BACKOFF = 600;


How did we came up with this number ? 600 Seconds seems to be too long. In a situation where the network is very restricted this would accumulate bulk of telemetry. Also is this configurable by channel in xml? If not it should be

Agreed, 10 minutes is a long time and it should be configurable. In the .NET implementation it will max out at 30 minutes in the backoff algorithm. Let's talk about this a bit more to see what we really want to do. By the time we reach this point we would have executed the backoff 15 times and probably have been throttled well over 15 minutes. It's possible this is just redundant.

@debugthings Yes, ideally I would want that to be configurable too. So instead of keeping that 15 constants for back of we have some sort of iterator pattern implemented which keeps on providing new backoff numbers until user specified limit. Internally iterator could well be implemented as Geometric Progression with ratio of 2. If we want to interleave with a constant time that could also be done.

grlima · 2018-02-12T18:10:01Z

...java/com/microsoft/applicationinsights/channel/concrete/inprocess/PartialSuccessHandler.java

+		if (args.getTransmission() != null && args.getTransmissionDispatcher() != null) {
+			switch (args.getResponseCode()) {
+			case HttpStatus.SC_PARTIAL_CONTENT:
+				BackendResponse beR = getBackendResponse(args.getResponseBody());


Nit picking: Please use a more descriptive name instead of beR. "backendResponse" would be fine.

Agreed, fixed.

grlima · 2018-02-12T18:12:31Z

...java/com/microsoft/applicationinsights/channel/concrete/inprocess/PartialSuccessHandler.java

+
+
+				List<String> newTransmission = new ArrayList<String>();
+				for (BackendResponse.Error e : beR.errors) {


null check missing for beR. I'd suggest checking for null right after line 39.

Fixed with logic update

dhaval24 · 2018-02-12T18:12:39Z

...n/java/com/microsoft/applicationinsights/internal/channel/common/TransmissionSendResult.java

@@ -24,7 +24,7 @@
 /**
 * Created by gupele on 2/10/2015.
 */
-enum TransmissionSendResult {
+public enum TransmissionSendResult {


Why do we need to make this enum public? Is it required for test cases? If yes please follow the Java Doc requirements and update the comment to be java doc comment with description.

Nah, must have been done via some Intellisense suggestion. Removed.

Haha, got it!

dhaval24

I have done second round of review and left my comments. I would like to have those changes. Further, I am mandating the need of java docs and being a little bit stricter on this because we are following this requirement for all new check-ins and is also mandatory for quality API

Moved the Handlers out of the concrete package to the common package to keep the same consistency. Removed a couple of unessecary methods. Added docs.

littleaj · 2018-02-12T22:09:28Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionNetworkOutput.java

+			} catch (ConnectionPoolTimeoutException e) {
+				ex = e;
+				InternalLogger.INSTANCE.error(
+						"Failed to send, connection pool timeout exception\r\n" + "Stack Trace:\r\n" + "%s",


Use %n and the format string chooses the correct EOL characters. Same with the rest of the log messages.
Also, let's remove the unnecessary concatenations; I don't know if JIT is smart enough to optimize this into one string literal. Let's assume it's not and merge these into one string.

Sounds good, eclipse auto format causes this most likely because I'm not using the %n string.

I looked it up and JIT is smart enough to turn those into one literal, so I'm less concerned with the concatenation. :)

littleaj · 2018-02-12T22:15:13Z

.../com/microsoft/applicationinsights/internal/channel/common/TransmissionFileSystemOutput.java

@@ -135,11 +135,13 @@ public TransmissionFileSystemOutput(String folderPath) {
    @Override
    public boolean send(Transmission transmission) {
        if (size.get() >= capacityInKB) {
+        	InternalLogger.INSTANCE.logAlways(InternalLogger.LoggingLevel.TRACE, "Persistent storage max capcity has been reached; currently at %s KB. Telemetry will be lost, please set the MaxTransmissionStorageFilesCapacityInMB property in the configuration file.", size.get());


I think this is a WARN level log. It's an exceptional case, but everything is still operational.

I think things are operational, however when this would happen it would indicate loss of telemetry. Imagine a banking application where this occurs and they loss transactional logs which were eventually needed for fraud analysis then this could be critical information. I would keep it as logalways and since this is rare I don't think it would any how effect production loads.

I'm suggesting changing it to InternalLogger.INSTANCE.logAlways(InternalLogger.LoggingLevel.WARN, ... I also think it should be logAlways.

Agreed, WARN makes more sense as it's something that is actionable but not ex

littleaj · 2018-02-12T22:15:58Z

.../com/microsoft/applicationinsights/internal/channel/common/TransmissionFileSystemOutput.java

            return false;
        }

        Optional<File> tempTransmissionFile = createTemporaryFile();
        if (!tempTransmissionFile.isPresent()) {
+        	InternalLogger.INSTANCE.logAlways(InternalLogger.LoggingLevel.TRACE, "Persistent storage unable to create temporary file.");


I think this is an ERROR level log. We can still run (i.e. it's not FATAL), but this component cannot do what it's designed to do.

I am removing this as it's redundant. There is a logger call in createTemporaryFile()

Sounds good.

littleaj · 2018-02-12T22:30:19Z

...in/java/com/microsoft/applicationinsights/internal/channel/common/PartialSuccessHandler.java

+						case HttpStatus.SC_REQUEST_TIMEOUT:
+						case HttpStatus.SC_INTERNAL_SERVER_ERROR:
+						case HttpStatus.SC_SERVICE_UNAVAILABLE:
+						case 429:


For the 429 and 439 case, can you append those lines with comments with a "friendly name" (similar to the constants in the previous cases)

Fixed with constants mentioned in other change.

grlima · 2018-02-12T23:05:10Z

...in/java/com/microsoft/applicationinsights/internal/channel/common/PartialSuccessHandler.java

+						case HttpStatus.SC_REQUEST_TIMEOUT:
+						case HttpStatus.SC_INTERNAL_SERVER_ERROR:
+						case HttpStatus.SC_SERVICE_UNAVAILABLE:
+						case 429:


Should we create constants for 429 and 439?

I think probably we should. It would allow when we extend the support later to support this codes.

HttpStatus is part of the Apache HttpClient. Will a comment be sufficient?

I think it makes more sense to have this as a constant. I modified TransmissionSendResult as it wasn't used anywhere else and was internal to the com.microsoft.applicationinsights.internal.channel.common package

dhaval24 · 2018-02-13T02:29:56Z

.../com/microsoft/applicationinsights/internal/channel/common/SenderThreadLocalBackOffData.java

-                 return instanceIsActive;
-            } catch (InterruptedException e) {
-                return false;
+            if (!instanceIsActive) {


@debugthings is it true that thread will never starve in the same state (i.e blocked state). If not we might always hit this condition for certain thread.

So, this is an artifact of the blocking backoff implementation (which I left intact). The only way for this to get set is if someone calls stop(). It's highly unlikely that it will be called since it's not referenced anywhere in the code.

It's in place to make sure if someone wants to disable the backoff they can do so with stop. Not surprisingly there is no start method so once the manager is stopped that's it

If there is a feature like this to disable the backoff it should be documented. And if there is no way to start if back then stop also doesn't make sense to me. Either case I think if we want to keep it just comment on it , regarding it is not advised to use this at any point or just remove the method if we do not plan to have this functionality.

I am all for deleting code that is not used.

I left this code intact as there are some old tests that are dependent on it and that is too much to tackle in this PR. Created task #559 to follow up

dhaval24 · 2018-02-13T02:44:24Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionNetworkOutput.java

+	private TransmissionNetworkOutput(String serverUri, TransmissionPolicyManager transmissionPolicyManager) {
+		Preconditions.checkNotNull(serverUri, "serverUri should be a valid non-null value");
+		Preconditions.checkArgument(!Strings.isNullOrEmpty(serverUri), "serverUri should be a valid non-null value");
+		Preconditions.checkNotNull(transmissionPolicyManager,


What are scenarios where this could be null. How does code flow happen in case of this event?

The Preconditions statements are for us. Callers of this method should make sure serverUri is not null before calling.

dhaval24

Looks good to me

AlexKlimovMS

Two minor comments

AlexKlimovMS · 2018-02-16T23:31:21Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionPolicyManager.java

+        	InternalLogger.INSTANCE.logAlways(InternalLogger.LoggingLevel.TRACE, "App is throttled, telemetry will be blocked for %s seconds.", backOffSeconds);
+        	this.suspendInSeconds(TransmissionPolicy.BACKOFF, backOffSeconds);
+        } else {
+        	// TODO Remove Static Time


Is this TODO still needed?

AlexKlimovMS · 2018-02-16T23:31:49Z

...ava/com/microsoft/applicationinsights/internal/channel/common/TransmissionPolicyManager.java

+
+    public void clearBackoff() {
+    	policyState.setCurrentState(TransmissionPolicy.UNBLOCKED);
+        backoffManager.onDoneSending();


Minor: extra spaces.

grlima · 2018-02-27T01:52:33Z

Replaced by #561

debugthings added 10 commits February 10, 2018 22:15

Initial commit of retry and backoff logic fixes

425e00f

Fixing warnings on files I touched this round

641ef58

Fix the eclipse UI from screaming about the docker Contstants

dc2fff7

Fixed backoff logic to use existing method. Added more logging to the…

efac9ce

… sender channel.

Added the partial response handler, more logging

9431ded

Added gson to core. Fixed backoff manager to keep original functional…

c2826ae

…ity. Added extension to return the timeout values as expected before.

Added unit tests.

094383a

Fixing string typed ArrayList<> to List<> per Dhaval

eb8eeb8

Missed one

cd32481

Making tests consistent.

d8edde3

dhaval24 suggested changes Feb 11, 2018

View reviewed changes

dhaval24 requested review from littleaj, SergeyKanzhelev, Dmitry-Matveev and grlima February 11, 2018 22:39

dhaval24 added the Enhancement label Feb 11, 2018

dhaval24 assigned debugthings Feb 11, 2018

dhaval24 added this to the 2.0.1 milestone Feb 11, 2018

debugthings added 2 commits February 12, 2018 10:22

Added javadoc comments, simplified logic for a few methods

6699afb

Added exception logging per @dhaval24. Fixed formatting on touched files

1e5d72c

dhaval24 changed the title ~~Implementing Retry logic~~ Implementing Retry logic [Reliable Channel] Feb 12, 2018

dhaval24 reviewed Feb 12, 2018

View reviewed changes

grlima reviewed Feb 12, 2018

View reviewed changes

dhaval24 reviewed Feb 12, 2018

View reviewed changes

dhaval24 suggested changes Feb 12, 2018

View reviewed changes

Updates per last round of commits

3a36811

Moved the Handlers out of the concrete package to the common package to keep the same consistency. Removed a couple of unessecary methods. Added docs.

littleaj suggested changes Feb 12, 2018

View reviewed changes

grlima reviewed Feb 12, 2018

View reviewed changes

dhaval24 reviewed Feb 13, 2018

View reviewed changes

Latest fixes

f917a83

dhaval24 approved these changes Feb 14, 2018

View reviewed changes

debugthings mentioned this pull request Feb 14, 2018

Implementing Retry logic [Reliable Channel] [STABLE Branch] #561

Merged

3 tasks

AlexKlimovMS reviewed Feb 16, 2018

View reviewed changes

grlima closed this Feb 27, 2018


		public final class TransmissionPolicyManager implements Stoppable, TransmissionHandlerObserver {

		private final int DEFAULT_MAX_SECONDS_TO_PAUSE_AFTER_MAX_BACKOFF = 600;



		List<String> newTransmission = new ArrayList<String>();
		for (BackendResponse.Error e : beR.errors) {

Implementing Retry logic [Reliable Channel] #556

Implementing Retry logic [Reliable Channel] #556

Conversation

debugthings commented Feb 11, 2018

dhaval24 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

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

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

dhaval24 Feb 12, 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

dhaval24 Feb 12, 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

dhaval24 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

littleaj Feb 12, 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

dhaval24 Feb 12, 2018 •

edited

Loading

dhaval24 Feb 12, 2018 •

edited

Loading

littleaj Feb 12, 2018 •

edited

Loading