-
Notifications
You must be signed in to change notification settings - Fork 223
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
[#1088] support graceful upper and down #1089
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1089 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 117 119 +2
Lines 2276 2290 +14
Branches 194 196 +2
======================================
- Misses 2276 2290 +14
☔ View full report in Codecov by Sentry. |
@@ -42,6 +42,10 @@ | |||
<groupId>com.huaweicloud</groupId> | |||
<artifactId>spring-cloud-huawei-router</artifactId> | |||
</dependency> | |||
<dependency> | |||
<groupId>org.springframework.boot</groupId> | |||
<artifactId>spring-boot-starter-actuator</artifactId> |
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.
Should be provided scope.
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.
fixed
@ConditionalOnNacosDiscoveryEnabled | ||
public class NacosGracefulAutoConfiguration { | ||
@Bean | ||
@ConditionalOnProperty(value = GovernanceProperties.NACOS_GRASEFUL_UPPER_DOWN, havingValue = "true") |
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.
Check if actuator relate class exists and enable on demand
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.
fixed
if (GovernanceProperties.GRASEFUL_STATUS_UPPER.equalsIgnoreCase(status)) { | ||
nacosDiscoveryProperties.setRegisterEnabled(true); | ||
nacosAutoServiceRegistration.start(); | ||
} else if (GovernanceProperties.GRASEFUL_STATUS_DOWN.equalsIgnoreCase(status)) { | ||
nacosServiceRegistry.deregister(nacosRegistration); | ||
} else { | ||
LOGGER.warn("status input " + status + " is not a valid value."); |
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.
Can registration be started/deploy for multi times? This case should carefully tested.
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.
add isRegistry for condition
return; | ||
} | ||
if (GovernanceProperties.GRASEFUL_STATUS_UPPER.equalsIgnoreCase(status)) { | ||
nacosDiscoveryProperties.setRegisterEnabled(true); |
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.
RegisterEnabled
可能产生二义性。 用户配置了这个配置项, 目的是不启用注册中心,还是需要使用这个特性? 在这个特性之前是前者。 所以使用该配置项, 会丢失特性
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.
registerEnabled是false的时候,AbstractAutoServiceRegistration的start方法中在第一个isEnabled()条件判断处直接返回,不会更新registion中的端口号,需要重新启动start方法去完成注册。
@@ -56,13 +60,14 @@ public void gracefulUpperAndDown(@Nullable String status) { | |||
if (StringUtils.isEmpty(status)) { | |||
return; | |||
} | |||
if (GovernanceProperties.GRASEFUL_STATUS_UPPER.equalsIgnoreCase(status)) { | |||
if (!isRegistry.get() && GovernanceProperties.GRASEFUL_STATUS_UPPER.equalsIgnoreCase(status)) { |
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.
use getAndSet here and check GovernanceProperties.GRASEFUL_STATUS_UPPER.equalsIgnoreCase(status) first
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.
fixed
nacosServiceRegistry.deregister(nacosRegistration); | ||
} else { | ||
LOGGER.warn("status input " + status + " is not a valid value."); | ||
LOGGER.warn("operation not allow, status: " + status); |
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.
is not allowed
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.
fixed
No description provided.