gaborkaszab commented on code in PR #14035: URL: https://github.com/apache/iceberg/pull/14035#discussion_r2379711095
########## api/src/main/java/org/apache/iceberg/exceptions/NotModifiedException.java: ########## @@ -0,0 +1,29 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.exceptions; + +import com.google.errorprone.annotations.FormatMethod; + +/** Exception to indicate that resource is unchanged. */ +public class NotModifiedException extends RESTException { Review Comment: Thanks for getting back with your thoughts, @amogh-jahagirdar ! I totally get your point that exceptions shouldn't be the way to control flow for non-error use cases. I did some digging around the area a bit and explored a couple of options. In summary what I think what makes it difficult for expressing non-200 codes from `RESTCatalogAdapter` is how we use it in our tests. There are 2 setups: 1) RESTSessionCatalog -> HttpClient -> RESTCatalogServlet -> RESTCatalogAdapter 2) RESTSessionCatalog -> RESTCatalogAdapter (basically mocking the server) For scenario 1) we have more flexibility because we could abstract whatever we return from `RESTCatalogAdapter` either in `RESTCatalogServlet` by converting it to HTTP status codes, or even in `HttpClient`. Starting here I [created some PoC](https://github.com/gaborkaszab/iceberg/commit/a1ca9020506cc119863f077612c7bdccc3610c84#diff-7b4ef726f1b6b2d636070e7d50b9acce6e9f95ae1af9283850d1f80a34c7ef08) where I return a new `LoadTableResponseWithStatusCode` from `RESTCatalogAdapter` and then this could be translated into a 304 on the servlet level without a response body. In turn in `HttpClient` I could translate such responses into a null response. However, the difficulty is scenario 2) where `RESTCatalogAdapter` is directly wired into `RESTSessionCatalog` as a client, because then whatever is returned from the adapter should be understood by the session catalog, including this new `...WithStatusCode` response. This isn't something I'd follow, because it would expose too much details to the session catalog that shouldn't be there. So long story short, I think we can't do anything fancy here because of the current design. I see 2 options: 1) The current one with exceptions, but then these exceptions have to be caught within `RESTSessionCatalog` too that could be somewhat misleading to the readers because it's a success-case driven by exception. 2) Return null from `RESTCatalogAdapter.handleRequest()`. With this, on `RESTSessionCatalog.loadTable()` side we have to make null checks and associate getting nulls meaning the table is unchanged. I don't think this is very intuitive. Btw, do you know the purpose of scenario 2) other than having simple tests mocking the communication with the server? I think it adds extra complexity for the reference IRC implementation a lot, and reduces the amount of complexity we have, e.g. with status code now. Would it make sense to get rid of such usage from the tests? -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
