-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
upgrade nacos client from 1.4.2 to 2.3.2 #12362
Changes from all commits
9a91c1d
c6f6705
ed0f4d8
73418bb
290b382
a95966a
9987735
99414f1
704187e
87e1c4a
0b65304
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 |
---|---|---|
|
@@ -35,32 +35,6 @@ | |
<dependency> | ||
<groupId>com.alibaba.nacos</groupId> | ||
<artifactId>nacos-client</artifactId> | ||
<exclusions> | ||
<exclusion> | ||
<groupId>org.slf4j</groupId> | ||
<artifactId>slf4j-api</artifactId> | ||
</exclusion> | ||
<exclusion> | ||
<groupId>org.apache.logging.log4j</groupId> | ||
<artifactId>log4j-core</artifactId> | ||
</exclusion> | ||
<exclusion> | ||
<groupId>org.apache.logging.log4j</groupId> | ||
<artifactId>log4j-slf4j-impl</artifactId> | ||
</exclusion> | ||
<exclusion> | ||
<groupId>ch.qos.logback</groupId> | ||
<artifactId>logback-classic</artifactId> | ||
</exclusion> | ||
<exclusion> | ||
<groupId>ch.qos.logback</groupId> | ||
<artifactId>logback-core</artifactId> | ||
</exclusion> | ||
<exclusion> | ||
<groupId>net.jcip</groupId> | ||
<artifactId>jcip-annotations</artifactId> | ||
</exclusion> | ||
</exclusions> | ||
Comment on lines
-38
to
-63
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 you exclude this? 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. nacos-client 2.3.2 have make these package optional, not transitivity |
||
</dependency> | ||
<!-- Override the dependency to use the same version of httpcore-nio --> | ||
<dependency> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,9 @@ | |
package org.apache.skywalking.oap.server.cluster.plugin.nacos; | ||
|
||
import com.alibaba.nacos.api.naming.NamingService; | ||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import lombok.Getter; | ||
import org.apache.skywalking.oap.server.core.cluster.ClusterCoordinator; | ||
import org.apache.skywalking.oap.server.core.cluster.ClusterNodesQuery; | ||
|
@@ -33,6 +36,7 @@ | |
import org.apache.skywalking.oap.server.telemetry.api.MetricsCreator; | ||
import org.apache.skywalking.oap.server.telemetry.none.MetricsCreatorNoop; | ||
import org.apache.skywalking.oap.server.telemetry.none.NoneTelemetryProvider; | ||
import org.junit.jupiter.api.AfterEach; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
import org.junit.jupiter.api.extension.ExtendWith; | ||
|
@@ -46,10 +50,6 @@ | |
import org.testcontainers.junit.jupiter.Testcontainers; | ||
import org.testcontainers.utility.DockerImageName; | ||
|
||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.List; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertFalse; | ||
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
@@ -58,17 +58,16 @@ | |
@ExtendWith(MockitoExtension.class) | ||
public class ClusterModuleNacosProviderFunctionalIT { | ||
|
||
private String nacosAddress; | ||
private final String username = "nacos"; | ||
private final String password = "nacos"; | ||
|
||
@Container | ||
public final GenericContainer<?> container = | ||
new GenericContainer<>(DockerImageName.parse("nacos/nacos-server:1.4.2")) | ||
new GenericContainer<>(DockerImageName.parse("nacos/nacos-server:v2.3.2-slim")) | ||
.waitingFor(Wait.forLogMessage(".*Nacos started successfully.*", 1)) | ||
.withEnv(Collections.singletonMap("MODE", "standalone")) | ||
.withExposedPorts(8848); | ||
|
||
.withLogConsumer(outputFrame -> System.out.print(outputFrame.getUtf8String())) | ||
.withExposedPorts(8848, 9848); | ||
private final String username = "nacos"; | ||
private final String password = "nacos"; | ||
private String nacosAddress; | ||
@Mock | ||
private ModuleManager moduleManager; | ||
@Mock | ||
|
@@ -82,6 +81,13 @@ public void before() { | |
Whitebox.setInternalState(telemetryModule, "loadedProvider", telemetryProvider); | ||
Mockito.when(moduleManager.find(TelemetryModule.NAME)).thenReturn(telemetryModule); | ||
nacosAddress = container.getHost() + ":" + container.getMappedPort(8848); | ||
Integer nacosPortOffset = container.getMappedPort(9848) - container.getMappedPort(8848); | ||
System.setProperty("nacos.server.grpc.port.offset", nacosPortOffset.toString()); | ||
Comment on lines
+84
to
+85
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. What is this about? 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. Nacos 2.x server have two port, 8848(http) and 9948(gRPC) by default. Nacos Client 's implement use 8848(http.port) + 1000(nacos.server.grpc.port.offset) as gRPC port. It works on virtual machine. Doc ref :https://nacos.io/docs/next/manual/user/java-sdk/properties/#24-%E8%BF%9E%E6%8E%A5%E7%9B%B8%E5%85%B3 |
||
} | ||
|
||
@AfterEach | ||
public void after() { | ||
System.clearProperty("nacos.server.grpc.port.offset"); | ||
} | ||
|
||
@Test | ||
|
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.
Why do we need to add this?
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.
module apache-skywalking-apm without dependencyManagement can not handle dependency conflict and version management
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.
oap-server has that, and dist itself is just packaging, it should not include any new dependency AFAIK.