amogh-jahagirdar commented on code in PR #14280: URL: https://github.com/apache/iceberg/pull/14280#discussion_r2425102595
########## core/src/main/java/org/apache/iceberg/util/Try.java: ########## @@ -0,0 +1,123 @@ +/* + * 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.util; + +import java.io.Serializable; +import org.apache.iceberg.relocated.com.google.common.base.Preconditions; + +/** + * Container for the result of an operation that might throw an exception. + * + * @param <T> the type of the result + */ +public class Try<T> implements Serializable { Review Comment: I feel like we can be even simpler to begin with here. I poked around locally, and I do agree we need some abstraction almost like a Try monad which encapsulates either a result of some kind or a failure. I originally was going to see if we could just use an abstraction over Tasks but ultimately, we need something to capture the state that it was a failure, not just handle the failure. We could use a separate AtomicReference<LocationProvider> and a boolean for indicating that a location provider could not be resolved, but that's pretty unclean. That said, I don't think we need a full blown public class with all these APIs, at least as of now. I think we could just have a package private class like the following with the main write API being `of(SerializableSupplier<T> supplier)` and the main read API being the `getOrThrow`. ``` class Try<T> implements Serializable { public static <T> of(SerializableSupplier<T> supplier) {//does what capture does} public static T getOrThrow().... } ``` I think this will remove the need for a separate test suite just for this class. Until we know we'll need this for a wider set of use cases, I think we should just keep this as minimal as possible. -- 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]
