Fix class loader issue for notifications response by joshuali925 · Pull Request #38 · opensearch-project/common-utils

joshuali925 · 2021-07-14T17:02:22Z

Signed-off-by: Joshua Li joshuali925@gmail.com

Description

fix from @dai-chen:
Recreate the response object from ActionResponse as specific notifications response in notifications plugin interface to avoid classloader issue when passing response between plugins

Issues Resolved

#37

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Joshua Li <joshuali925@gmail.com>

dai-chen

Thanks for the fix!

dblock

I love this as a start.

Try to refactor this where you don't need to override fun onResponse manually in every instance.

I think I want to be able to write this:

wrapper.execute(
    SEND_NOTIFICATION_ACTION_TYPE,
    SendNotificationRequest(eventSource, channelMessage, channelIds, threadContext),
    object: SomeNewType<ActionResponse, SendNotificationResponse>
}

dblock

This should be refactored, and definitely needs tests to be merged.

dai-chen · 2021-07-14T20:27:20Z

I love this as a start.

Try to refactor this where you don't need to override fun onResponse manually in every instance.

I think I want to be able to write this:
wrapper.execute(
    SEND_NOTIFICATION_ACTION_TYPE,
    SendNotificationRequest(eventSource, channelMessage, channelIds, threadContext),
    object: SomeNewType<ActionResponse, SendNotificationResponse>
}

I was trying to extract a wrapActionListener method though had problem with recreateObject() function which requires reified type (generic type didn't work). Let me give it another try. Thanks!

Signed-off-by: Joshua Li <joshuali925@gmail.com>

dblock · 2021-07-14T22:15:00Z

@joshuali925 excellent!

Is it worth putting this into a method (e.g. executeAction) that takes DELETE_NOTIFICATION_CONFIG_ACTION_TYPE, and DeleteNotificationConfigResponse or are we trying too hard?

client.execute(
            DELETE_NOTIFICATION_CONFIG_ACTION_TYPE,
            request,
            listener
            wrapActionListener(listener) { response -> recreateObject(response) { DeleteNotificationConfigResponse(it) } }
        )

I almost think that maybe this entire implementation is a map of DELETE_NOTIFICATION_CONFIG_ACTION_TYPE -> DeleteNotificationConfigResponse, but it's getting very meta. 🤔

Signed-off-by: Joshua Li <joshuali925@gmail.com>

joshuali925 · 2021-07-14T22:56:33Z

@dblock agree that could look cleaner, but in sendNotification the client is wrapped with SecureClientWrapper before calling execute. I assume we'll need to make this an exception if we generalize the execute function, and i'm not sure if there will be other exceptions in the future. Anyways I'll also defer to @dai-chen since he is coming up with the fixes and refactoring

common-utils/src/main/kotlin/org/opensearch/commons/notifications/NotificationsPluginInterface.kt

Lines 218 to 225 in d5bcf10

    
           val threadContext: String? = 
        
               client.threadPool().threadContext.getTransient<String>(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT) 
        
           val wrapper = SecureClientWrapper(client) // Executing request in privileged mode 
        
           wrapper.execute( 
        
               SEND_NOTIFICATION_ACTION_TYPE, 
        
               SendNotificationRequest(eventSource, channelMessage, channelIds, threadContext), 
        
               wrapActionListener(listener) { response -> recreateObject(response) { SendNotificationResponse(it) } } 
        
           )

dai-chen · 2021-07-19T21:48:48Z

Closing for #40

dai-chen · 2021-07-19T21:51:51Z

@dblock agree that could look cleaner, but in sendNotification the client is wrapped with SecureClientWrapper before calling execute. I assume we'll need to make this an exception if we generalize the execute function, and i'm not sure if there will be other exceptions in the future. Anyways I'll also defer to @dai-chen since he is coming up with the fixes and refactoring

common-utils/src/main/kotlin/org/opensearch/commons/notifications/NotificationsPluginInterface.kt

Lines 218 to 225 in d5bcf10

val threadContext: String? =

client.threadPool().threadContext.getTransient<String>(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT)

val wrapper = SecureClientWrapper(client) // Executing request in privileged mode

wrapper.execute(

SEND_NOTIFICATION_ACTION_TYPE,

SendNotificationRequest(eventSource, channelMessage, channelIds, threadContext),

wrapActionListener(listener) { response -> recreateObject(response) { SendNotificationResponse(it) } }

)

@joshuali925 @dblock I tried this idea but the code was not clean as I thought. This is mostly because of generic types and number fo params required by each execute call. I would suggest keeping it as is for now.

joshuali925 added 2 commits July 14, 2021 09:55

Fix class loader issue for notifications

7a6a11f

Signed-off-by: Joshua Li <joshuali925@gmail.com>

Fix formatting

98383cf

Signed-off-by: Joshua Li <joshuali925@gmail.com>

joshuali925 added the bug Something isn't working label Jul 14, 2021

dai-chen previously approved these changes Jul 14, 2021

View reviewed changes

dblock reviewed Jul 14, 2021

View reviewed changes

dblock suggested changes Jul 14, 2021

View reviewed changes

dblock mentioned this pull request Jul 14, 2021

Fix type cast issue in transport action call opensearch-project/notifications#223

Merged

6 tasks

Refactor creation of action listener object

269a5d4

Signed-off-by: Joshua Li <joshuali925@gmail.com>

joshuali925 dismissed dai-chen’s stale review via 269a5d4 July 14, 2021 21:34

Fix indentation

d5bcf10

Signed-off-by: Joshua Li <joshuali925@gmail.com>

Remove unused suppresses

165dbba

Signed-off-by: Joshua Li <joshuali925@gmail.com>

dai-chen mentioned this pull request Jul 19, 2021

Fix class loader issue for notifications response #40

Merged

5 tasks

dai-chen closed this Jul 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix class loader issue for notifications response#38

Fix class loader issue for notifications response#38
joshuali925 wants to merge 5 commits intoopensearch-project:mainfrom
joshuali925:notifications-classloader

joshuali925 commented Jul 14, 2021

Uh oh!

dai-chen left a comment

Uh oh!

dblock left a comment

Uh oh!

dblock left a comment

Uh oh!

dai-chen commented Jul 14, 2021 •

edited

Loading

Uh oh!

dblock commented Jul 14, 2021 •

edited

Loading

Uh oh!

joshuali925 commented Jul 14, 2021

Uh oh!

dai-chen commented Jul 19, 2021

Uh oh!

dai-chen commented Jul 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

joshuali925 commented Jul 14, 2021

Description

Issues Resolved

Check List

Uh oh!

dai-chen left a comment

Choose a reason for hiding this comment

Uh oh!

dblock left a comment

Choose a reason for hiding this comment

Uh oh!

dblock left a comment

Choose a reason for hiding this comment

Uh oh!

dai-chen commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dblock commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshuali925 commented Jul 14, 2021

Uh oh!

dai-chen commented Jul 19, 2021

Uh oh!

dai-chen commented Jul 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dai-chen commented Jul 14, 2021 •

edited

Loading

dblock commented Jul 14, 2021 •

edited

Loading