-
Notifications
You must be signed in to change notification settings - Fork 352
Implement intermediate data support for events (based on #3391) #3380
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
Implement intermediate data support for events (based on #3391) #3380
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.
Thanks, for your contribution, @rishii-19-works ! I'm adding extra reviewers, who, I hope, are more familiar with the affected code.
| CreateGenericTableRequest createGenericTableRequest, | ||
| RealmContext realmContext, | ||
| SecurityContext securityContext) { | ||
|
|
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.
nit: these whitespace change appear to be irrelevant to the purpose of the PR. Since the PR does not attempt to refactor code, why introduce whitespace changes? IMHO, they complicate review.
| delegate.createGenericTable( | ||
| prefix, namespace, createGenericTableRequest, realmContext, securityContext); | ||
|
|
||
| LoadGenericTableResponse responseEntity = (LoadGenericTableResponse) resp.getEntity(); |
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.
same here: why extract a variable when the PR's intention is to "document" ? 🤔
|
@adnanhemani : FYI |
|
I'm sorry, I'm not sure I see any utility in any part of this PR. The point of #3209 is not to "document" anything, but to come up with a solution. Anyone who reads and understands the code is aware of this limitation - documentation does not change anything. |
dimas-b
left a comment
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.
-1 Blocking for now due to irrelevant changes.
| // generalized it to support non-aws S3 implementations | ||
| @JsonIgnore public static final String ROLE_ARN_PATTERN = "^.+:(.*):iam:.*:(.*):role/.+$"; | ||
| @JsonIgnore | ||
| public static final String ROLE_ARN_PATTERN = "^.+:(.*):iam:.*:(.*):role/.+$"; |
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.
-1 This change is irrelevant to the purpose of this PR stated in the description.
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.
Thanks for the review. I understand the concern about the irrelevant changes.
I’ll clean up the PR to keep it strictly scoped to the intended change and remove any unrelated modifications. I’ll update the PR shortly.
|
Closing this draft PR to rework it based on the approach proposed in #3391. I’ll open a new PR once aligned. |
This PR is a draft toward implementing #3209 and is based on the approach proposed in the POC in #3391. It explores a mechanism to carry intermediate data produced during the request lifecycle into the event system, without tying events strictly to response DTOs. The intent is to allow request-scoped intermediate POJOs to be made available at event emission time, following the same design direction as the original proposal, while refining it toward a production-ready implementation.