-
Notifications
You must be signed in to change notification settings - Fork 38
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
Retry / File State Storage #385
Conversation
…daptive card Merge in SYMPHONYP/symphony-java-toolkit from feature/SYMPHONYP-939-ms-teams-spring-bot-retry-messages to spring-bot-master-db * commit '3f2fec632e13056b0fec79bffe62697fec0e4913': SYMPHONYP-940 MS Teams feature to submit the adaptive card
…:8082/scm/symphonyp/symphony-java-toolkit into feature/SYMPHONYP-939-ms-teams-spring-bot-retry-messages
Merge in SYMPHONYP/symphony-java-toolkit from feature/SYMPHONYP-939-ms-teams-spring-bot-retry-messages to spring-bot-master-db * commit '3e4b60cbfa4318e45122d113b408aec181c8bf98': review comments changes done. added testcases for Message Retry. SYMPHONYP-939 implements review comments SYMPHONYP-940 refactor retry handler fixing formatting Adding roadmap Update LICENSE setting copyright name
@@ -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; |
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
@@ -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 comment
The 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 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.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
please make sure we have code coverage of this.
Merge in SYMPHONYP/symphony-java-toolkit from bugfix/SYMPHONYP-946-History-Api-bug-fix to spring-bot-master-db * commit '18d766fd56c9bb561b2de3179c75cd57fae5e3eb': bug fix implemented.
…andler refactor code Merge in SYMPHONYP/symphony-java-toolkit from feature/SYMPHONYP-947-teams-spring-bot-retry-handler-refactor-code to spring-bot-master-db * commit '320488b2b314041fab0498f244763a1c2250973c': refactor retry message handler refactor retry message handler refactor retry message handler refactor retry message handler refactor retry message handler refactor retry message handler
@@ -72,6 +72,28 @@ public void testStoreWithTagDates() throws IOException { | |||
Assertions.assertEquals(2, hoover(tss.retrieve(tagList4, false)).size()); | |||
} | |||
|
|||
@Test | |||
public void testSlashStoreWithMultTags() throws IOException { |
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.
testFilenamesWithSlash
tss.store("thefile/thefile", tags1, somedata); | ||
tss.store("thefile/theotherfile", tags2, somedata); | ||
|
||
Assertions.assertEquals(1, hoover(tss.retrieve(tagList1, false)).size()); |
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.
shouldn't you also check you can retrieve the other file as well?
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.
ok.. I will add other file also.
|
||
public class InMemoryRetryingActivityHandler extends AbstractRetryingActivityHandler { | ||
|
||
private Queue<MessageRetry> queue = new ConcurrentLinkedQueue<>(); |
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.
Have you looked at this? https://stackoverflow.com/questions/40485398/retry-logic-with-completablefuture
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.
I have already tried this, it was not working. As per our requirement if exception occurred we need to wait for Retry-After then try retry.
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.
What didn't work about it? Is it the lack of delay? I am looking at this code from the SO page:
public CompletableFuture<Result> executeActionAsync() {
CompletableFuture<Result> f=executeMycustomActionHere();
for(int i=0; i<MAX_RETRIES; i++) {
f=f.thenApply(CompletableFuture::completedFuture)
.exceptionally(t -> executeMycustomActionHere())
.thenCompose(Function.identity());
}
return f;
}
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.
should not be too hard to add a delay on the .exceptionally() line, if that is the problem
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.
No our requirement is to wait for Retry-Afer time, but we can't add the sleep or wait as it will wait the thread. we have implemented similar only inserted data into queue. and added cron job to resent the data.
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.
we just don't want to used Thread.sleep(ra); in code. As it will keep the current thread on wait. :)
We can connect on Monday, I will explain you.
I am learning the CompletableFuture :)
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.
argh you're right!! 😦
How about this: deutschebank#1
I actually wrote a test here - well, changed your InMemoryRetryingActivityHandlerTest
to see if this actually worked.
Test passes but maybe you have more to add..?
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.
Hi Rob,
Testcase only pass, if we have for (int i = 0; i < teamsRetryCount; i++) {
for loop in code. But in our case, if exception occurred we simply insert that record in in-memory queue.
Here we have to check the how to write testcase around schedule also.
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.
Hi, we can wait until Wednesday to talk about this or we can maybe have a call a bit sooner?
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.
Hi Rob, we will connect tomorrow. Completely overload with teams.
I have tried CompletableFuture.delayedExecutor(ra, TimeUnit.MILLISECONDS) but it is in java 9, so we can't go for it.
Alternate solution i found as scheduler.
https://stackoverflow.com/questions/58707689/is-it-possible-to-schedule-a-completablefuture
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.
looks good to me
refactor retry handler