bharos commented on code in PR #10641:
URL: https://github.com/apache/gravitino/pull/10641#discussion_r3024764061
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java:
##########
@@ -579,7 +579,7 @@ static LoadTableResponse
filterSnapshotsByRefs(LoadTableResponse loadTableRespon
* @param loadTableResponse the table response to include in the body
* @return a Response with ETag header set
*/
- private static Response buildResponseWithETag(LoadTableResponse
loadTableResponse) {
+ static Response buildResponseWithETag(LoadTableResponse loadTableResponse) {
Review Comment:
IcebergRESTUtils already exists for response-building helpers,
buildResponseWithETag can belong there ? And can be shared between
IcebergTableOperations and IcebergNamespaceOperations
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/rest/IcebergTableOperations.java:
##########
@@ -579,7 +579,7 @@ static LoadTableResponse
filterSnapshotsByRefs(LoadTableResponse loadTableRespon
* @param loadTableResponse the table response to include in the body
* @return a Response with ETag header set
*/
- private static Response buildResponseWithETag(LoadTableResponse
loadTableResponse) {
+ static Response buildResponseWithETag(LoadTableResponse loadTableResponse) {
Review Comment:
We are making this method from private to package-private to be invoked from
icebergNamespaceOperations, which seems to be a bit of anti-pattern
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/rest/TestIcebergNamespaceOperations.java:
##########
@@ -189,6 +190,16 @@ void testRegisterTable() {
verifyRegisterTableSucc("register_foo2", Namespace.of("register_ns_2",
"a"));
}
+ @Test
+ void testRegisterTableReturnsETag() {
+ Namespace ns = Namespace.of("register_etag_ns");
+ Response response = doRegisterTable("register_etag_foo1", ns);
+ Assertions.assertEquals(Response.Status.OK.getStatusCode(),
response.getStatus());
+ String etag = response.getHeaderString("ETag");
+ Assertions.assertNotNull(etag, "ETag header should be present in register
table response");
Review Comment:
Is it also possible to check the ETag match?
Test only checks ETag presence, not validity Since the mock has a known
metadata location (/mock/metadata/v1.metadata.json), you could compute the
expected SHA-256 and assert the exact value
--
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]
