-
Notifications
You must be signed in to change notification settings - Fork 35
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
Changes from 8 commits
3f2fec6
25a538f
a9f3699
2d448ea
80509b3
b9a31ff
3e4b60c
1781441
18d766f
add1a4d
4b9e848
5c475ca
f49836b
a5496fe
e2c1e8e
320488b
be97480
115d93e
eb8de4a
8f82361
3bf0ea7
784b2c7
7e21035
9007a16
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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(); | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -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; | ||
|
||
public TeamsResponseHandler( | ||
AttachmentHandler attachmentHandler, | ||
|
@@ -78,7 +77,7 @@ public TeamsResponseHandler( | |
this.displayTemplater = displayTemplater; | ||
this.teamsState = th; | ||
this.teamsConversations = tc; | ||
this.messageRetryHandler = mr; | ||
this.retryHandler = mr; | ||
} | ||
|
||
protected void initErrorHandler() { | ||
|
@@ -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(); | ||
|
||
|
@@ -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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's odd that this has changed? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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){ | ||
|
@@ -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(); | ||
|
@@ -274,7 +251,7 @@ public void retryMessage() { | |
int messageCount = 0; | ||
|
||
Optional<MessageRetry> opt; | ||
while ((opt = messageRetryHandler.get()).isPresent()) { | ||
while ((opt = retryHandler.get()).isPresent()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()); | ||
} | ||
|
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; | ||
} | ||
|
||
|
||
} |
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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