-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-9146: KIP-571 Add option to force delete active members in StreamsResetter #8589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
8f00c6f
617dcf6
e02c3c6
c6b4ef3
6a845f3
6b15ded
d43ce29
1853cfe
d88ad33
4764677
eb50b5d
91c81b6
6c5778a
6dedd1c
f015c22
f92a3f6
87346c0
174ba7e
5329315
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -182,7 +182,6 @@ | |
| import org.apache.kafka.common.requests.FindCoordinatorResponse; | ||
| import org.apache.kafka.common.requests.IncrementalAlterConfigsRequest; | ||
| import org.apache.kafka.common.requests.IncrementalAlterConfigsResponse; | ||
| import org.apache.kafka.common.requests.JoinGroupRequest; | ||
| import org.apache.kafka.common.requests.LeaveGroupRequest; | ||
| import org.apache.kafka.common.requests.LeaveGroupResponse; | ||
| import org.apache.kafka.common.requests.ListGroupsRequest; | ||
|
|
@@ -3612,6 +3611,27 @@ private boolean dependsOnSpecificNode(ConfigResource resource) { | |
| || resource.type() == ConfigResource.Type.BROKER_LOGGER; | ||
| } | ||
|
|
||
| private List<MemberIdentity> getMembersFromGroup(String groupId) { | ||
| Collection<MemberDescription> members = new ArrayList<>(); | ||
| try { | ||
| members = describeConsumerGroups(Collections.singleton(groupId)).describedGroups().get(groupId).get().members(); | ||
| } catch (Throwable ex) { | ||
| System.out.println("Encounter exception when trying to get members from group: " + groupId); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove print statements
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed~ |
||
| ex.printStackTrace(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious why we are still continuing in this case, as the member lookup already fails.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, will fix this .
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
| } | ||
|
|
||
| List<MemberIdentity> memberToRemove = new ArrayList<>(); | ||
| for (MemberDescription member: members) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style error here. I would recommend doing a self style check like:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the advice, will fix it in the next commit.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reran the self style check, but didn't capture any error. I assume the error would be the missed |
||
| if (member.groupInstanceId().isPresent()) { | ||
| memberToRemove.add(new MemberIdentity().setGroupInstanceId(member.groupInstanceId().get()) | ||
| ); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This indentation is a bit weird, let's just merge L3625-3626
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
| } else { | ||
| memberToRemove.add(new MemberIdentity().setMemberId(member.consumerId())); | ||
| } | ||
| } | ||
| return memberToRemove; | ||
| } | ||
|
|
||
| @Override | ||
| public RemoveMembersFromConsumerGroupResult removeMembersFromConsumerGroup(String groupId, | ||
| RemoveMembersFromConsumerGroupOptions options) { | ||
|
|
@@ -3621,24 +3641,37 @@ public RemoveMembersFromConsumerGroupResult removeMembersFromConsumerGroup(Strin | |
| KafkaFutureImpl<Map<MemberIdentity, Errors>> future = new KafkaFutureImpl<>(); | ||
|
|
||
| ConsumerGroupOperationContext<Map<MemberIdentity, Errors>, RemoveMembersFromConsumerGroupOptions> context = | ||
| new ConsumerGroupOperationContext<>(groupId, options, deadline, future); | ||
| new ConsumerGroupOperationContext<>(groupId, options, deadline, future); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's get back the original indentation.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
|
|
||
| Call findCoordinatorCall = getFindCoordinatorCall(context, | ||
| () -> getRemoveMembersFromGroupCall(context)); | ||
| Call findCoordinatorCall; | ||
| if (options.removeAll()) { | ||
| List<MemberIdentity> members = getMembersFromGroup(groupId); | ||
| findCoordinatorCall = getFindCoordinatorCall(context, | ||
| () -> getRemoveMembersFromGroupCall(context, members)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we pass the members into the context?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My initial thought was to put the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I feel this is more consistent for internal calls not to do a second round of interpretation for which There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And to be clear, I'm not suggesting we have to put stuff into the context, just always passing in the intended removal list and do not depend on |
||
| } else { | ||
| findCoordinatorCall = getFindCoordinatorCall(context, | ||
| () -> getRemoveMembersFromGroupCall(context, new ArrayList<>())); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be Collection.emptyList() |
||
| } | ||
| runnable.call(findCoordinatorCall, startFindCoordinatorMs); | ||
|
|
||
| return new RemoveMembersFromConsumerGroupResult(future, options.members()); | ||
| return new RemoveMembersFromConsumerGroupResult(future, options.members(), options.removeAll()); | ||
| } | ||
|
|
||
| private Call getRemoveMembersFromGroupCall(ConsumerGroupOperationContext<Map<MemberIdentity, Errors>, RemoveMembersFromConsumerGroupOptions> context) { | ||
| private Call getRemoveMembersFromGroupCall(ConsumerGroupOperationContext<Map<MemberIdentity, Errors>, | ||
| RemoveMembersFromConsumerGroupOptions> context, List<MemberIdentity> allMembers) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we could name it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, fixed |
||
| return new Call("leaveGroup", | ||
| context.deadline(), | ||
| new ConstantNodeIdProvider(context.node().get().id())) { | ||
| @Override | ||
| LeaveGroupRequest.Builder createRequest(int timeoutMs) { | ||
| return new LeaveGroupRequest.Builder(context.groupId(), | ||
| context.options().members().stream().map( | ||
| MemberToRemove::toMemberIdentity).collect(Collectors.toList())); | ||
| if (context.options().removeAll()) { | ||
| return new LeaveGroupRequest.Builder(context.groupId(), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we could merge L3666-3667
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||
| allMembers); | ||
| } else { | ||
| return new LeaveGroupRequest.Builder(context.groupId(), | ||
| context.options().members().stream().map( | ||
| MemberToRemove::toMemberIdentity).collect(Collectors.toList())); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -3647,7 +3680,7 @@ void handleResponse(AbstractResponse abstractResponse) { | |
|
|
||
| // If coordinator changed since we fetched it, retry | ||
| if (ConsumerGroupOperationContext.hasCoordinatorMoved(response)) { | ||
| Call call = getRemoveMembersFromGroupCall(context); | ||
| Call call = getRemoveMembersFromGroupCall(context, allMembers); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we blindly put
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed, now we explicitly pass in the members to be deleted to the private |
||
| rescheduleFindCoordinatorTask(context, () -> call, this); | ||
| return; | ||
| } | ||
|
|
@@ -3660,7 +3693,7 @@ void handleResponse(AbstractResponse abstractResponse) { | |
| // We set member.id to empty here explicitly, so that the lookup will succeed as user doesn't | ||
| // know the exact member.id. | ||
| memberErrors.put(new MemberIdentity() | ||
| .setMemberId(JoinGroupRequest.UNKNOWN_MEMBER_ID) | ||
| .setMemberId(memberResponse.memberId()) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could see this doesn't hold true for a plain static member removal. Let's discuss why skipping the individual member check in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For removing static members, this still true because we put memberId as
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if I understand the change. Also not sure if I can follow the comments. Can you elaborate? |
||
| .setGroupInstanceId(memberResponse.groupInstanceId()), | ||
| Errors.forCode(memberResponse.errorCode())); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,12 +32,23 @@ | |
| public class RemoveMembersFromConsumerGroupOptions extends AbstractOptions<RemoveMembersFromConsumerGroupOptions> { | ||
|
|
||
| private Set<MemberToRemove> members; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we just make members to be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. Taking a step further, can we just keep the the type
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated~ |
||
| private boolean removeAll; | ||
|
|
||
| public RemoveMembersFromConsumerGroupOptions(Collection<MemberToRemove> members) { | ||
| this.members = new HashSet<>(members); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we have different semantics for an empty collection (it was "remove nothing" originally, and we change it to "remove all"), I am wondering if we should do a check if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sense. It will throw exception if empty members provided now. |
||
| this.removeAll = false; | ||
| } | ||
|
|
||
| public RemoveMembersFromConsumerGroupOptions(Boolean removeAll) { | ||
| this.members = new HashSet<>(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Collections.emptySet() makes more sense since it is immutable.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sense, fixed |
||
| this.removeAll = removeAll; | ||
| } | ||
|
|
||
| public Set<MemberToRemove> members() { | ||
| return members; | ||
| } | ||
|
|
||
| public boolean removeAll() { | ||
| return removeAll; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,11 +33,14 @@ public class RemoveMembersFromConsumerGroupResult { | |
|
|
||
| private final KafkaFuture<Map<MemberIdentity, Errors>> future; | ||
| private final Set<MemberToRemove> memberInfos; | ||
| private final boolean removeAll; | ||
|
|
||
| RemoveMembersFromConsumerGroupResult(KafkaFuture<Map<MemberIdentity, Errors>> future, | ||
| Set<MemberToRemove> memberInfos) { | ||
| Set<MemberToRemove> memberInfos, | ||
| Boolean removeAll) { | ||
| this.future = future; | ||
| this.memberInfos = memberInfos; | ||
| this.removeAll = removeAll; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -46,20 +49,33 @@ public class RemoveMembersFromConsumerGroupResult { | |
| * If not, the first member error shall be returned. | ||
| */ | ||
| public KafkaFuture<Void> all() { | ||
| final KafkaFutureImpl<Void> result = new KafkaFutureImpl<>(); | ||
| this.future.whenComplete((memberErrors, throwable) -> { | ||
| if (throwable != null) { | ||
| result.completeExceptionally(throwable); | ||
| } else { | ||
| for (MemberToRemove memberToRemove : memberInfos) { | ||
| if (maybeCompleteExceptionally(memberErrors, memberToRemove.toMemberIdentity(), result)) { | ||
| return; | ||
| if (removeAll) { | ||
| final KafkaFutureImpl<Void> result = new KafkaFutureImpl<>(); | ||
| this.future.whenComplete((memberErrors, throwable) -> { | ||
| if (throwable != null) { | ||
| result.completeExceptionally(throwable); | ||
| } else { | ||
| System.out.println("Remove all active members succeeded, removed " + memberErrors.size() + " members: " + memberErrors.keySet()); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove print statement.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
| result.complete(null); | ||
| } | ||
| }); | ||
| return result; | ||
| } else { | ||
| final KafkaFutureImpl<Void> result = new KafkaFutureImpl<>(); | ||
| this.future.whenComplete((memberErrors, throwable) -> { | ||
| if (throwable != null) { | ||
| result.completeExceptionally(throwable); | ||
| } else { | ||
| for (MemberToRemove memberToRemove : memberInfos) { | ||
| if (maybeCompleteExceptionally(memberErrors, memberToRemove.toMemberIdentity(), result)) { | ||
| return; | ||
| } | ||
| } | ||
| result.complete(null); | ||
| } | ||
| result.complete(null); | ||
| } | ||
| }); | ||
| return result; | ||
| }); | ||
| return result; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2411,6 +2411,50 @@ public void testRemoveMembersFromGroup() throws Exception { | |
| assertNull(noErrorResult.all().get()); | ||
| assertNull(noErrorResult.memberResult(memberOne).get()); | ||
| assertNull(noErrorResult.memberResult(memberTwo).get()); | ||
|
|
||
| // Return with success for "removeAll" scenario | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test looks good, but it seems that we didn't test the case where some members get deleted successfully while some are not?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! Added the test for partial failure |
||
| // 1. KafkaAdminClient should query all members successfully | ||
| // 1.1 construct the DescribeGroupsResponse | ||
| TopicPartition myTopicPartition0 = new TopicPartition("my_topic", 0); | ||
| TopicPartition myTopicPartition1 = new TopicPartition("my_topic", 1); | ||
| TopicPartition myTopicPartition2 = new TopicPartition("my_topic", 2); | ||
|
|
||
| final List<TopicPartition> topicPartitions = new ArrayList<>(); | ||
| topicPartitions.add(0, myTopicPartition0); | ||
| topicPartitions.add(1, myTopicPartition1); | ||
| topicPartitions.add(2, myTopicPartition2); | ||
|
|
||
| final ByteBuffer memberAssignment = ConsumerProtocol.serializeAssignment(new ConsumerPartitionAssignor.Assignment(topicPartitions)); | ||
| byte[] memberAssignmentBytes = new byte[memberAssignment.remaining()]; | ||
| DescribedGroupMember groupInstanceOne = DescribeGroupsResponse.groupMember("0", instanceOne, "clientId0", "clientHost", memberAssignmentBytes, null); | ||
| DescribedGroupMember groupInstanceTwo = DescribeGroupsResponse.groupMember("1", instanceTwo, "clientId1", "clientHost", memberAssignmentBytes, null); | ||
| DescribeGroupsResponseData data = new DescribeGroupsResponseData(); | ||
| data.groups().add(DescribeGroupsResponse.groupMetadata( | ||
| groupId, | ||
| Errors.NONE, | ||
| "", | ||
| ConsumerProtocol.PROTOCOL_TYPE, | ||
| "", | ||
| asList(groupInstanceOne, groupInstanceTwo), | ||
| Collections.emptySet())); | ||
|
|
||
| // 1.2 prepare response for AdminClient.describeConsumerGroups | ||
| env.kafkaClient().prepareResponse(prepareFindCoordinatorResponse(Errors.NONE, env.cluster().controller())); | ||
| env.kafkaClient().prepareResponse(new DescribeGroupsResponse(data)); | ||
|
|
||
| // 2. KafkaAdminClient should delete all members correctly | ||
| env.kafkaClient().prepareResponse(prepareFindCoordinatorResponse(Errors.NONE, env.cluster().controller())); | ||
| env.kafkaClient().prepareResponse(new LeaveGroupResponse( | ||
| new LeaveGroupResponseData().setErrorCode(Errors.NONE.code()).setMembers( | ||
| Arrays.asList(responseTwo, | ||
| new MemberResponse().setGroupInstanceId(instanceOne).setErrorCode(Errors.NONE.code()) | ||
| )) | ||
| )); | ||
| final RemoveMembersFromConsumerGroupResult successResult = env.adminClient().removeMembersFromConsumerGroup( | ||
| groupId, | ||
| new RemoveMembersFromConsumerGroupOptions(true) | ||
| ); | ||
| assertNull(successResult.all().get()); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ | |
| import org.apache.kafka.clients.admin.DescribeConsumerGroupsOptions; | ||
| import org.apache.kafka.clients.admin.DescribeConsumerGroupsResult; | ||
| import org.apache.kafka.clients.admin.MemberDescription; | ||
| import org.apache.kafka.clients.admin.RemoveMembersFromConsumerGroupOptions; | ||
| import org.apache.kafka.clients.consumer.Consumer; | ||
| import org.apache.kafka.clients.consumer.ConsumerConfig; | ||
| import org.apache.kafka.clients.consumer.KafkaConsumer; | ||
|
|
@@ -106,6 +107,7 @@ public class StreamsResetter { | |
| private static OptionSpec versionOption; | ||
| private static OptionSpecBuilder executeOption; | ||
| private static OptionSpec<String> commandConfigOption; | ||
| private static OptionSpec forceOption; | ||
|
|
||
| private static String usage = "This tool helps to quickly reset an application in order to reprocess " | ||
| + "its data from scratch.\n" | ||
|
|
@@ -149,7 +151,7 @@ public int run(final String[] args, | |
| properties.put(ConsumerConfig.BOOTSTRAP_SERVERS_CONFIG, options.valueOf(bootstrapServerOption)); | ||
|
|
||
| adminClient = Admin.create(properties); | ||
| validateNoActiveConsumers(groupId, adminClient); | ||
| maybeDeleteActiveConsumers(groupId, adminClient); | ||
|
|
||
| allTopics.clear(); | ||
| allTopics.addAll(adminClient.listTopics().names().get(60, TimeUnit.SECONDS)); | ||
|
|
@@ -176,8 +178,8 @@ public int run(final String[] args, | |
| return exitCode; | ||
| } | ||
|
|
||
| private void validateNoActiveConsumers(final String groupId, | ||
| final Admin adminClient) | ||
| private void maybeDeleteActiveConsumers(final String groupId, | ||
| final Admin adminClient) | ||
| throws ExecutionException, InterruptedException { | ||
|
|
||
| final DescribeConsumerGroupsResult describeResult = adminClient.describeConsumerGroups( | ||
|
|
@@ -186,9 +188,15 @@ private void validateNoActiveConsumers(final String groupId, | |
| final List<MemberDescription> members = | ||
| new ArrayList<>(describeResult.describedGroups().get(groupId).get().members()); | ||
| if (!members.isEmpty()) { | ||
| throw new IllegalStateException("Consumer group '" + groupId + "' is still active " | ||
| + "and has following members: " + members + ". " | ||
| + "Make sure to stop all running application instances before running the reset tool."); | ||
| if (options.has(forceOption)) { | ||
| System.out.println("Force deleting all active members in the group: " + groupId); | ||
| adminClient.removeMembersFromConsumerGroup(groupId, new RemoveMembersFromConsumerGroupOptions(true)).all(); | ||
| } else { | ||
| throw new IllegalStateException("Consumer group '" + groupId + "' is still active " | ||
| + "and has following members: " + members + ". " | ||
| + "Make sure to stop all running application instances before running the reset tool." + | ||
|
feyman2016 marked this conversation as resolved.
Outdated
|
||
| "Try set '--force' in the cmdline to force delete active members."); | ||
|
feyman2016 marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -236,6 +244,8 @@ private void parseArguments(final String[] args) { | |
| .withRequiredArg() | ||
| .ofType(String.class) | ||
| .describedAs("file name"); | ||
| forceOption = optionParser.accepts("force", "Force remove members when long session time out has been configured, " + | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we also want to edit the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so, updated
feyman2016 marked this conversation as resolved.
Outdated
|
||
| "please make sure to shut down all stream applications when this option is specified to avoid unexpected rebalances."); | ||
|
feyman2016 marked this conversation as resolved.
Outdated
|
||
| executeOption = optionParser.accepts("execute", "Execute the command."); | ||
| dryRunOption = optionParser.accepts("dry-run", "Display the actions that would be performed without executing the reset commands."); | ||
| helpOption = optionParser.accepts("help", "Print usage information.").forHelp(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should catch
Exceptionhere:https://stackoverflow.com/questions/2274102/difference-between-using-throwable-and-exception-in-a-try-catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense, fixed~