Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry / File State Storage #385

Merged
merged 24 commits into from
May 17, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
3f2fec6
SYMPHONYP-940 MS Teams feature to submit the adaptive card
vaibhav-db Feb 24, 2023
25a538f
Pull request #29: SYMPHONYP-940 MS Teams feature to submit the adapti…
vaibhav-db Feb 27, 2023
a9f3699
SYMPHONYP-940 refactor retry handler
vaibhav-db Mar 2, 2023
2d448ea
Merge branch 'spring-bot-master' of https://stash.gto.intranet.db.com…
vaibhav-db Mar 2, 2023
80509b3
SYMPHONYP-939 implements review comments
vaibhav-db Mar 15, 2023
b9a31ff
added testcases for Message Retry.
abhishekdahiya-db Mar 16, 2023
3e4b60c
review comments changes done.
abhishekdahiya-db Mar 21, 2023
1781441
Pull request #30: SYMPHONYP-940 refactor retry handler
vaibhav-db Mar 27, 2023
18d766f
bug fix implemented.
abhishekdahiya-db Mar 31, 2023
add1a4d
refactor retry message handler
vaibhav-db Mar 31, 2023
4b9e848
refactor retry message handler
vaibhav-db Mar 31, 2023
5c475ca
refactor retry message handler
vaibhav-db Mar 31, 2023
f49836b
Pull request #31: bug fix implemented.
abhishekdahiya-db Apr 3, 2023
a5496fe
refactor retry message handler
vaibhav-db Apr 4, 2023
e2c1e8e
refactor retry message handler
vaibhav-db Apr 4, 2023
320488b
refactor retry message handler
vaibhav-db Apr 5, 2023
be97480
Pull request #32: Feature/SYMPHONYP-947 teams spring bot retry handle…
vaibhav-db Apr 5, 2023
115d93e
Fixed state storage testcases
vaibhav-db Apr 7, 2023
eb8de4a
State storage testcases.
vaibhav-db Apr 14, 2023
8f82361
refactor file stage storage and testcase
vaibhav-db Apr 21, 2023
3bf0ea7
Teams retry InMemory handler
vaibhav-db Apr 27, 2023
784b2c7
Teams retry InMemory handler
vaibhav-db Apr 27, 2023
7e21035
Corrected InMemoryRetryingActivityHandler reference
vaibhav-db May 2, 2023
9007a16
Error stream id in log
vaibhav-db May 3, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,23 +1,41 @@
package org.finos.springbot.teams;

import java.util.concurrent.TimeUnit;

import org.finos.springbot.teams.handlers.TeamsResponseHandler;
import org.finos.springbot.teams.handlers.retry.NoOpRetryHandler;
import org.finos.springbot.teams.handlers.retry.MessageRetryHandler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.scheduling.annotation.EnableScheduling;
import org.springframework.scheduling.annotation.Scheduled;

import org.springframework.scheduling.annotation.SchedulingConfigurer;
import org.springframework.scheduling.config.ScheduledTaskRegistrar;

@EnableScheduling
public class TeamsScheduledConfig {
public class TeamsScheduledConfig implements SchedulingConfigurer {

private static final Logger LOG = LoggerFactory.getLogger(TeamsScheduledConfig.class);

@Autowired
private TeamsResponseHandler handler;

@Scheduled(fixedDelay = 30, timeUnit = TimeUnit.SECONDS)
//Task to run after a fixed delay.
//the duration between the end of last execution and the start of next execution is fixed
public void scheduleRetryMessage() {

@Autowired
private MessageRetryHandler retryHandler;

@Value("${teams.retry.time:30000}")
private long teamsRetrySchedulerCron;

@Override
public void configureTasks(ScheduledTaskRegistrar scheduledTaskRegistrar) {
if (retryHandler instanceof NoOpRetryHandler) {
LOG.info("No-operation retry handler is configured.");
} else {
Runnable runnable = () -> scheduleRetryMessage();
scheduledTaskRegistrar.addFixedDelayTask(runnable, teamsRetrySchedulerCron);
}
}

private void scheduleRetryMessage() {
handler.retryMessage();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@
import org.finos.springbot.teams.conversations.TeamsConversationsConfig;
import org.finos.springbot.teams.form.TeamsFormConverter;
import org.finos.springbot.teams.form.TeamsFormDeserializerModule;
import org.finos.springbot.teams.handlers.InMemoryMessageRetryHandler;
import org.finos.springbot.teams.handlers.MessageRetryHandler;
import org.finos.springbot.teams.handlers.TeamsResponseHandler;
import org.finos.springbot.teams.handlers.retry.MessageRetryHandler;
import org.finos.springbot.teams.handlers.retry.NoOpRetryHandler;
import org.finos.springbot.teams.history.StateStorageBasedTeamsHistory;
import org.finos.springbot.teams.history.StorageIDResponseHandler;
import org.finos.springbot.teams.history.TeamsHistory;
Expand Down Expand Up @@ -230,10 +230,14 @@ public void setResourceLoaderClassLoader() {
resourceLoader.setClassLoader(this.getClass().getClassLoader());
}

/**
* set InMemoryMessageRetryHandler() if you want retry
* @return
*/
@Bean
@ConditionalOnMissingBean
public MessageRetryHandler messageRetryHandler() {
return new InMemoryMessageRetryHandler();
return new NoOpRetryHandler();
}

}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,17 @@
import java.util.concurrent.CompletionException;
import java.util.function.BiFunction;

import org.apache.commons.lang3.StringUtils;
import org.finos.springbot.teams.TeamsException;
import org.finos.springbot.teams.content.TeamsAddressable;
import org.finos.springbot.teams.conversations.TeamsConversations;
import org.finos.springbot.teams.handlers.retry.MessageRetry;
import org.finos.springbot.teams.handlers.retry.MessageRetryHandler;
import org.finos.springbot.teams.history.StorageIDResponseHandler;
import org.finos.springbot.teams.history.TeamsHistory;
import org.finos.springbot.teams.response.templating.EntityMarkupTemplateProvider;
import org.finos.springbot.teams.response.templating.MarkupAndEntities;
import org.finos.springbot.teams.state.TeamsStateStorage;
import org.finos.springbot.teams.templating.adaptivecard.AdaptiveCardPassthrough;
import org.finos.springbot.teams.templating.adaptivecard.AdaptiveCardTemplateProvider;
import org.finos.springbot.teams.templating.thymeleaf.ThymeleafTemplateProvider;
import org.finos.springbot.workflow.annotations.WorkMode;
Expand All @@ -31,7 +33,6 @@
import org.springframework.beans.BeansException;
import org.springframework.context.ApplicationContext;
import org.springframework.context.ApplicationContextAware;
import org.springframework.http.HttpStatus;
import org.springframework.util.ErrorHandler;

import com.fasterxml.jackson.core.JsonProcessingException;
Expand All @@ -45,13 +46,11 @@
import com.microsoft.bot.schema.ResourceResponse;
import com.microsoft.bot.schema.TextFormatTypes;

import okhttp3.ResponseBody;

public class TeamsResponseHandler implements ResponseHandler, ApplicationContextAware {

private static final Logger LOG = LoggerFactory.getLogger(TeamsResponseHandler.class);

private static final int RETRY_COUNT = 3;

private static final int INIT_RETRY_COUNT = 0;

protected AttachmentHandler attachmentHandler;
Expand All @@ -62,7 +61,7 @@ public class TeamsResponseHandler implements ResponseHandler, ApplicationContext
protected ThymeleafTemplateProvider displayTemplater;
protected TeamsStateStorage teamsState;
protected TeamsConversations teamsConversations;
protected MessageRetryHandler messageRetryHandler;
protected MessageRetryHandler retryHandler;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we not have retryHandler in a subclass? e.g. RetryingTeamsResponseHandler?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes done as per review comments


public TeamsResponseHandler(
AttachmentHandler attachmentHandler,
Expand All @@ -78,7 +77,7 @@ public TeamsResponseHandler(
this.displayTemplater = displayTemplater;
this.teamsState = th;
this.teamsConversations = tc;
this.messageRetryHandler = mr;
this.retryHandler = mr;
}

protected void initErrorHandler() {
Expand All @@ -94,7 +93,7 @@ public void accept(Response t) {
sendResponse(t, INIT_RETRY_COUNT);
}

private void sendResponse(Response t, int retryCount) {
public void sendResponse(Response t, int retryCount) {
if (t.getAddress() instanceof TeamsAddressable) {
TeamsAddressable ta = (TeamsAddressable) t.getAddress();

Expand Down Expand Up @@ -142,7 +141,7 @@ protected TemplateType getTemplateType(WorkResponse wr) {
TemplateType tt;
if (displayTemplater.hasTemplate(wr)) {
tt = TemplateType.THYMELEAF;
} else if (workTemplater.hasTemplate(wr)) {
} else if (workTemplater.hasTemplate(wr) || AdaptiveCardPassthrough.isAdaptiveCard(wr)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's odd that this has changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Rob,

We have usecase where we wanted to send adaptive card as passthrough like user send Adaptive card json as string input parameter and bot will send this adaptive card to ms teams channel.

tt = TemplateType.ADAPTIVE_CARD;
} else if (wr.getMode() == WorkMode.EDIT) {
tt = TemplateType.ADAPTIVE_CARD;
Expand Down Expand Up @@ -191,32 +190,10 @@ private BiFunction<? super ResourceResponse, Throwable, ResourceResponse> handle
};
}

private boolean insertIntoQueue(Response t, int retryCount, Throwable e) {
if (e instanceof CompletionException
&& ((CompletionException) e).getCause() instanceof ErrorResponseException) {
ErrorResponseException ere = (ErrorResponseException) ((CompletionException) e).getCause();
retrofit2.Response<ResponseBody> response = ere.response();
if (response.code() == HttpStatus.TOO_MANY_REQUESTS.value() && retryCount <= RETRY_COUNT) {
String retryAfter = response.headers().get("Retry-After");

int retryAfterInt = 1;//initiate to 1 sec
if(StringUtils.isNumeric(retryAfter)) {
retryAfterInt = Integer.parseInt(retryAfter);
}

messageRetryHandler.add(new MessageRetry(t, retryCount, retryAfterInt));

return true;
}
}

return false;
}

private BiFunction<? super ResourceResponse, Throwable, ResourceResponse> handleErrorAndStorage(Object out, TeamsAddressable address, Map<String, Object> data, Response t, int retryCount) {
return (rr, e) -> {
if (e != null) {
boolean success = insertIntoQueue(t, retryCount, e);
boolean success = retryHandler.handleException(t, retryCount, e);
if(!success) {
LOG.error(e.getMessage());
if (out instanceof ObjectNode){
Expand All @@ -238,7 +215,7 @@ private BiFunction<? super ResourceResponse, Throwable, ResourceResponse> handle
return null;
};
}

protected CompletableFuture<ResourceResponse> sendCardResponse(JsonNode json, TeamsAddressable address, Map<String, Object> data) throws Exception {
Activity out = Activity.createMessageActivity();
Attachment body = new Attachment();
Expand Down Expand Up @@ -274,7 +251,7 @@ public void retryMessage() {
int messageCount = 0;

Optional<MessageRetry> opt;
while ((opt = messageRetryHandler.get()).isPresent()) {
while ((opt = retryHandler.get()).isPresent()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make sure we have code coverage of this.

messageCount++;
this.sendResponse(opt.get().getResponse(), opt.get().getRetryCount());
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package org.finos.springbot.teams.handlers.retry;

import java.time.LocalDateTime;
import java.util.concurrent.CompletionException;

import org.apache.commons.lang3.StringUtils;
import org.finos.springbot.workflow.response.Response;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.http.HttpStatus;

import com.microsoft.bot.connector.rest.ErrorResponseException;

import okhttp3.ResponseBody;

public abstract class BasicMessageRetryHandler implements MessageRetryHandler {

private static final Logger LOG = LoggerFactory.getLogger(BasicMessageRetryHandler.class);

@Value("${teams.retry.count:3}")
private long teamsRetryCount;

public boolean handleException(Response t, int retryCount, Throwable e) {
if (e instanceof CompletionException
&& ((CompletionException) e).getCause() instanceof ErrorResponseException) {
ErrorResponseException ere = (ErrorResponseException) ((CompletionException) e).getCause();
retrofit2.Response<ResponseBody> response = ere.response();
if (response.code() == HttpStatus.TOO_MANY_REQUESTS.value() && retryCount <= teamsRetryCount) {
String retryAfter = response.headers().get("Retry-After");
LOG.info("MessageRetryHandler request retryAfter {}", retryAfter);

int retryAfterInt = 1;//initiate to 1 sec
if(StringUtils.isNumeric(retryAfter)) {
retryAfterInt = Integer.parseInt(retryAfter);
}
LocalDateTime time = LocalDateTime.now().plusSeconds(retryAfterInt);
add(new MessageRetry(t, retryCount, retryAfterInt, time));

return true;
}
}

return false;
}


}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package org.finos.springbot.teams.handlers;
package org.finos.springbot.teams.handlers.retry;

import java.time.LocalDateTime;
import java.util.Optional;
Expand All @@ -8,31 +8,36 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class InMemoryMessageRetryHandler implements MessageRetryHandler {
public class InMemoryMessageRetryHandler extends BasicMessageRetryHandler {

private static final Logger LOG = LoggerFactory.getLogger(InMemoryMessageRetryHandler.class);

Queue<MessageRetry> queue = new ConcurrentLinkedQueue<>();
private Queue<MessageRetry> queue = new ConcurrentLinkedQueue<>();

@Override
public void add(MessageRetry t) {
queue.add(t);
}

@Override
public void clearAll() {
while(queue.poll()!=null);
}

@Override
public Optional<MessageRetry> get() {
MessageRetry q;
if ((q = queue.peek()) != null) {
LocalDateTime time = q.getCurrentTime().plusSeconds(q.getRetryAfter());
if (LocalDateTime.now().isAfter(time)) { // retry now
if (LocalDateTime.now().isAfter(q.getRetryTime())) { // retry now
queue.remove(q);
return Optional.of(q);
}else {
LOG.info("Message not retied, as retry after time {} is not getter than or equal to current time {}", time, LocalDateTime.now());
LOG.info("Message not retried, as retry time {} for message has not passed the current time {}", q.getRetryTime(), LocalDateTime.now());
}
}

return Optional.empty();
}


}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package org.finos.springbot.teams.handlers;
package org.finos.springbot.teams.handlers.retry;

import java.time.LocalDateTime;

Expand All @@ -9,14 +9,14 @@ public class MessageRetry {
private Response response;
private int retryCount;
private int retryAfter;
private LocalDateTime currentTime;
private LocalDateTime retryTime;

public MessageRetry(Response response, int retryCount, int retryAfter) {
public MessageRetry(Response response, int retryCount, int retryAfter, LocalDateTime retryTime) {
super();
this.response = response;
this.retryCount = retryCount;
this.retryAfter = retryAfter;
currentTime = LocalDateTime.now();
this.retryTime = retryTime;
}

public Response getResponse() {
Expand Down Expand Up @@ -44,25 +44,25 @@ public void setRetryAfter(int retryAfter) {
}


public LocalDateTime getCurrentTime() {
return currentTime;
public LocalDateTime getRetryTime() {
return retryTime;
}

public void setCurrentTime(LocalDateTime currentTime) {
this.currentTime = currentTime;
public void setRetryTime(LocalDateTime retryTime) {
this.retryTime = retryTime;
}

@Override
public String toString() {
return "MessageRetry [response=" + response + ", retryCount=" + retryCount + ", retryAfter=" + retryAfter
+ ", localDate=" + currentTime + "]";
+ ", localDate=" + retryTime + "]";
}

@Override
public int hashCode() {
final int prime = 31;
int result = 1;
result = prime * result + ((currentTime == null) ? 0 : currentTime.hashCode());
result = prime * result + ((retryTime == null) ? 0 : retryTime.hashCode());
result = prime * result + ((response == null) ? 0 : response.hashCode());
result = prime * result + retryAfter;
result = prime * result + retryCount;
Expand All @@ -78,10 +78,10 @@ public boolean equals(Object obj) {
if (getClass() != obj.getClass())
return false;
MessageRetry other = (MessageRetry) obj;
if (currentTime == null) {
if (other.currentTime != null)
if (retryTime == null) {
if (other.retryTime != null)
return false;
} else if (!currentTime.equals(other.currentTime))
} else if (!retryTime.equals(other.retryTime))
return false;
if (response == null) {
if (other.response != null)
Expand Down
Loading