Re: [PR] [#5518] feat(iceberg): add http header information to Iceberg Event [gravitino]
FANNG1 commented on code in PR #5495:
URL: https://github.com/apache/gravitino/pull/5495#discussion_r1835573239
##
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java:
##
@@ -269,6 +269,10 @@ public Response reportTableMetrics(
return IcebergRestUtils.noContent();
}
+ HttpServletRequest httpServletRequest() {
Review Comment:
update the comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5518] feat(iceberg): add http header information to Iceberg Event [gravitino]
FANNG1 commented on code in PR #5495:
URL: https://github.com/apache/gravitino/pull/5495#discussion_r1835569333
##
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRequestContext.java:
##
@@ -19,16 +19,23 @@
package org.apache.gravitino.iceberg.service;
+import java.util.Map;
import javax.servlet.http.HttpServletRequest;
import lombok.Getter;
+import org.apache.gravitino.utils.PrincipalUtils;
/** The general request context information for Iceberg REST operations. */
public class IcebergRequestContext {
- @Getter private final HttpServletRequest httpRequest;
+
@Getter private final String catalogName;
+ @Getter private final String userName;
+ @Getter private final String remoteIp;
Review Comment:
remoteHostName and remoteIP contains same information, I perfer just keep
one, do you think remoteHostName is more useful than remoteIP?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5518] feat(iceberg): add http header information to Iceberg Event [gravitino]
puchengy commented on PR #5495: URL: https://github.com/apache/gravitino/pull/5495#issuecomment-2465290151 @FANNG1 Thank you, LGTM, left some nits. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5518] feat(iceberg): add http header information to Iceberg Event [gravitino]
puchengy commented on PR #5495: URL: https://github.com/apache/gravitino/pull/5495#issuecomment-2465289562 nit, the title of the PR should be expanded to include ip address (and possibly remote hostname if my suggestion is implemented). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5518] feat(iceberg): add http header information to Iceberg Event [gravitino]
puchengy commented on code in PR #5495:
URL: https://github.com/apache/gravitino/pull/5495#discussion_r1834750272
##
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRequestContext.java:
##
@@ -19,16 +19,23 @@
package org.apache.gravitino.iceberg.service;
+import java.util.Map;
import javax.servlet.http.HttpServletRequest;
import lombok.Getter;
+import org.apache.gravitino.utils.PrincipalUtils;
/** The general request context information for Iceberg REST operations. */
public class IcebergRequestContext {
- @Getter private final HttpServletRequest httpRequest;
+
@Getter private final String catalogName;
+ @Getter private final String userName;
+ @Getter private final String remoteIp;
Review Comment:
May I ask if we can add remoteHostName as well?
##
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java:
##
@@ -269,6 +269,10 @@ public Response reportTableMetrics(
return IcebergRestUtils.noContent();
}
+ HttpServletRequest httpServletRequest() {
Review Comment:
Curious why do we need this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5518] feat(iceberg): add http header information to Iceberg Event [gravitino]
FANNG1 commented on PR #5495: URL: https://github.com/apache/gravitino/pull/5495#issuecomment-2464008230 After this PR is merged, you could add Http header information to Gravitino server event too, and then make audit frame work to generate audit log for both Gravitino server and Gravitino IcebergREST server. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5518] feat(iceberg): add http header information to Iceberg Event [gravitino]
FANNG1 commented on PR #5495: URL: https://github.com/apache/gravitino/pull/5495#issuecomment-2463898622 @jerryshao @puchengy it's ready to review now, please help to review again. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
