Thanks for your PR. I think this looks good to me. Thanks, Zike Yang
On Mon, Jan 30, 2023 at 4:23 PM Enrico Olivelli <eolive...@gmail.com> wrote: > > I totally agree. > There has been little API Design error to add that "throws" clause. > > Thanks for the initiative > > Enrico > > Il giorno lun 30 gen 2023 alle ore 09:16 <mattisonc...@gmail.com> ha scritto: > > > > Hello, everyone > > > > I submitted this PR https://github.com/apache/pulsar/pull/19356 to discuss > > if we can accept moving this checked exception into the builder to avoid > > adding more useless try-catch blocks. > > > > In practice, we should create the new transaction like this: > > > > ```java > > final TransactionBuilder transactionBuilder; > > try { > > transactionBuilder = client.newTransaction(); > > } catch (PulsarClientException e) { > > // handle this exception. > > return; > > } > > CompletableFuture<Transaction> txnFuture = transactionBuilder > > .withTransactionTimeout(1, TimeUnit.MINUTES) > > .build(); > > ``` > > But this exception only throws by > > > > ```java > > public TransactionBuilder newTransaction() throws PulsarClientException { > > if (!conf.isEnableTransaction()) { > > throw new > > PulsarClientException.InvalidConfigurationException("Transactions are not > > enabled"); > > } > > return new TransactionBuilderImpl(this, tcClient); > > } > > ``` > > Therefore, even if we enabled the transaction, we still need to handle this > > checked exception. Maybe we can move this checked exception into the > > builder or make this exception to be runtime to avoid writing the more > > useless try-catch blocks. > > > > ```java > > client.newTransaction() > > .withTransactionTimeout(1, TimeUnit.MINUTES) > > .build() > > .thenCompose(transaction -> { > > // do somethings > > }).exceptionally(ex -> { > > // handle exception > > }); > > ``` > > It is better, isn't it? > > > > Best, > > Mattison
