-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fix #341: Optimization: Callbacks invocation #1595
Conversation
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.
Good job. Please update also the documentation. I have added several comments to consider.
...src/main/java/io/getlime/security/powerauth/app/server/configuration/AsyncConfiguration.java
Outdated
Show resolved
Hide resolved
...ava/io/getlime/security/powerauth/app/server/database/model/converter/DurationConverter.java
Outdated
Show resolved
Hide resolved
...n/java/io/getlime/security/powerauth/app/server/database/model/entity/CallbackUrlEntity.java
Show resolved
Hide resolved
...a/io/getlime/security/powerauth/app/server/database/model/entity/CallbackUrlEventEntity.java
Outdated
Show resolved
Hide resolved
...io/getlime/security/powerauth/app/server/database/repository/CallbackUrlEventRepository.java
Outdated
Show resolved
Hide resolved
...a/io/getlime/security/powerauth/app/server/service/callbacks/CallbackUrlEventDispatcher.java
Outdated
Show resolved
Hide resolved
...th-java-server/src/main/java/io/getlime/security/powerauth/app/server/task/CleaningTask.java
Outdated
Show resolved
Hide resolved
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.
Good job. I have several concerns, especially related to performance and data migration which should be handled or answered.
docs/db/changelog/changesets/powerauth-java-server/1.8.x/20240704-callback-event-table.xml
Outdated
Show resolved
Hide resolved
docs/db/changelog/changesets/powerauth-java-server/1.8.x/20240704-callback-event-table.xml
Outdated
Show resolved
Hide resolved
...src/main/java/io/getlime/security/powerauth/app/server/configuration/AsyncConfiguration.java
Outdated
Show resolved
Hide resolved
.../io/getlime/security/powerauth/app/server/configuration/PowerAuthCallbacksConfiguration.java
Outdated
Show resolved
Hide resolved
...n/java/io/getlime/security/powerauth/app/server/database/model/entity/CallbackUrlEntity.java
Show resolved
Hide resolved
...io/getlime/security/powerauth/app/server/database/repository/CallbackUrlEventRepository.java
Outdated
Show resolved
Hide resolved
...io/getlime/security/powerauth/app/server/database/repository/CallbackUrlEventRepository.java
Outdated
Show resolved
Hide resolved
...io/getlime/security/powerauth/app/server/database/repository/CallbackUrlEventRepository.java
Outdated
Show resolved
Hide resolved
...a/io/getlime/security/powerauth/app/server/service/callbacks/CallbackUrlEventDispatcher.java
Outdated
Show resolved
Hide resolved
...a/io/getlime/security/powerauth/app/server/service/callbacks/CallbackUrlEventDispatcher.java
Outdated
Show resolved
Hide resolved
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.
Great, almost done. Let's polish it a little bit.
docs/db/changelog/changesets/powerauth-java-server/1.8.x/20240704-callback-event-table.xml
Outdated
Show resolved
Hide resolved
.../io/getlime/security/powerauth/app/server/configuration/PowerAuthCallbacksConfiguration.java
Outdated
Show resolved
Hide resolved
...ava/io/getlime/security/powerauth/app/server/database/model/converter/DurationConverter.java
Show resolved
Hide resolved
...a/io/getlime/security/powerauth/app/server/database/model/entity/CallbackUrlEventEntity.java
Outdated
Show resolved
Hide resolved
...th-java-server/src/main/java/io/getlime/security/powerauth/app/server/task/CleaningTask.java
Outdated
Show resolved
Hide resolved
...th-java-server/src/main/java/io/getlime/security/powerauth/app/server/task/CleaningTask.java
Outdated
Show resolved
Hide resolved
...io/getlime/security/powerauth/app/server/database/repository/CallbackUrlEventRepository.java
Outdated
Show resolved
Hide resolved
...io/getlime/security/powerauth/app/server/database/repository/CallbackUrlEventRepository.java
Outdated
Show resolved
Hide resolved
...th-java-server/src/main/java/io/getlime/security/powerauth/app/server/task/CleaningTask.java
Outdated
Show resolved
Hide resolved
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. @korbelm @zcgandcomp May we do performance testing from this branch before merging?
...src/main/java/io/getlime/security/powerauth/app/server/configuration/AsyncConfiguration.java
Outdated
Show resolved
Hide resolved
...ava/io/getlime/security/powerauth/app/server/service/callbacks/CallbackUrlEventListener.java
Outdated
Show resolved
Hide resolved
...java/io/getlime/security/powerauth/app/server/service/callbacks/CallbackUrlEventService.java
Outdated
Show resolved
Hide resolved
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 OK
...io/getlime/security/powerauth/app/server/database/repository/CallbackUrlEventRepository.java
Outdated
Show resolved
Hide resolved
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 suggest we run load tests on this branch before merging. Otherwise I approve the changes.
5c6cdce
to
7fa9064
Compare
Coming with a significant changes due to inability to use the used locking mechanism on MSSQL. Now the approach works as follows:
On an activation or operation change a new
|
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.
In general, looks good. I have added several ideas to consider. Did it pass the performance tests? Let's wait for other reviews.
docs/db/changelog/changesets/powerauth-java-server/1.9.x/20240704-callback-event-table.xml
Outdated
Show resolved
Hide resolved
.../io/getlime/security/powerauth/app/server/configuration/PowerAuthCallbacksConfiguration.java
Outdated
Show resolved
Hide resolved
* @param error Exception describing the cause of failure. | ||
*/ | ||
@Transactional(propagation = Propagation.REQUIRES_NEW) | ||
public void handleFailure(final CallbackUrlEvent callbackUrlEvent, final Throwable error) { |
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.
When I see Throwable
, I am curious whether we are supposed to handle even Error
. I suggest to avoid that.
...getlime/security/powerauth/app/server/service/callbacks/CallbackUrlEventResponseHandler.java
Show resolved
Hide resolved
...getlime/security/powerauth/app/server/service/callbacks/CallbackUrlEventResponseHandler.java
Outdated
Show resolved
Hide resolved
*/ | ||
private RestClient getRestClient(final CallbackUrlEventConfiguration callbackUrlEventConfiguration) throws RestClientException { | ||
final String cacheKey = getRestClientCacheKey(callbackUrlEventConfiguration); | ||
synchronized (restClientCacheLock) { |
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.
Let's run the Coverity scan after merging this. Last time, we overlooked something.
...er/src/main/java/io/getlime/security/powerauth/app/server/service/util/TransactionUtils.java
Outdated
Show resolved
Hide resolved
...getlime/security/powerauth/app/server/database/model/enumeration/CallbackUrlEventStatus.java
Show resolved
Hide resolved
...getlime/security/powerauth/app/server/database/model/enumeration/CallbackUrlEventStatus.java
Show resolved
Hide resolved
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 found several minor additional issues.
docs/db/changelog/changesets/powerauth-java-server/1.9.x/20240704-callback-event-table.xml
Outdated
Show resolved
Hide resolved
docs/db/changelog/changesets/powerauth-java-server/1.9.x/20240704-callback-event-table.xml
Outdated
Show resolved
Hide resolved
docs/db/changelog/changesets/powerauth-java-server/1.9.x/20240704-callback-event-table.xml
Outdated
Show resolved
Hide resolved
...java/io/getlime/security/powerauth/app/server/service/callbacks/CallbackUrlEventService.java
Outdated
Show resolved
Hide resolved
40c39cc
to
d7e68e0
Compare
Fix #341 by implementing the Outbox Pattern.
Instead of dealing with callback sending in the business logic, an
CallbackUrlEvent
is created and published for an event listener. The listener, implemented inCallbackUrlEventDispatcher.class
, then processes theCallbackUrlEvent
in a separate thread and transaction.