You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2021/09/14 04:42:24 UTC

[GitHub] [skywalking] wankai123 opened a new pull request #7709: Support gRPC sync grouped dynamic configurations.

wankai123 opened a new pull request #7709:
URL: https://github.com/apache/skywalking/pull/7709


   ### Support gRPC sync grouped dynamic configurations.
   - [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
   - [X] Update the documentation to include this new feature.
   - [X] Tests(including UT, IT, E2E) are added to verify the new feature.
   - [ ] If it's UI related, attach the screenshots below.
   
   - [X] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #7081.
   - [X] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/CHANGES.md).
   


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [skywalking] wankai123 commented on a change in pull request #7709: Support gRPC sync grouped dynamic configurations.

Posted by GitBox <gi...@apache.org>.
wankai123 commented on a change in pull request #7709:
URL: https://github.com/apache/skywalking/pull/7709#discussion_r707952576



##########
File path: oap-server/server-configuration/grpc-configuration-sync/src/main/test/java/org/apache/skywalking/oap/server/configuration/grpc/GRPCConfigurationTest.java
##########
@@ -0,0 +1,177 @@
+/*

Review comment:
       :sweat:




-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #7709: Support gRPC sync grouped dynamic configurations.

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7709:
URL: https://github.com/apache/skywalking/pull/7709#discussion_r707931124



##########
File path: oap-server/server-configuration/grpc-configuration-sync/src/main/test/java/org/apache/skywalking/oap/server/configuration/grpc/GRPCConfigurationTest.java
##########
@@ -0,0 +1,177 @@
+/*

Review comment:
       src/main/test is typically wrong




-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #7709: Support gRPC sync grouped dynamic configurations.

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7709:
URL: https://github.com/apache/skywalking/pull/7709#discussion_r707981567



##########
File path: oap-server/server-configuration/grpc-configuration-sync/src/main/java/org/apache/skywalking/oap/server/configuration/grpc/GRPCConfigWatcherRegister.java
##########
@@ -70,14 +70,42 @@ public GRPCConfigWatcherRegister(RemoteEndpointSettings settings) {
             });
             this.uuid = responseUuid;
         } catch (Exception e) {
-            LOGGER.error("Remote config center [" + settings + "] is not available.", e);
+            log.error("Remote config center [{}] is not available.", settings, e);
         }
         return Optional.of(table);
     }
 
     @Override
     public Optional<GroupConfigTable> readGroupConfig(final Set<String> keys) {
-        // TODO: implement readGroupConfig
-        return Optional.empty();
+        GroupConfigTable groupConfigTable = new GroupConfigTable();
+        try {
+            ConfigurationRequest.Builder builder = ConfigurationRequest.newBuilder()
+                                                                       .setClusterName(settings.getClusterName());
+            if (groupUuid != null) {
+                builder.setUuid(groupUuid);
+            }
+            GroupConfigurationResponse response = stub.callGroup(builder.build());
+            String responseUuid = response.getUuid();
+            if (Objects.equals(groupUuid, responseUuid)) {
+                // If UUID matched, the config table is expected as empty.
+                return Optional.empty();
+            }
+
+            response.getGroupConfigTableList().forEach(rspGroupConfigItems -> {
+                String groupName = rspGroupConfigItems.getGroupName();
+                if (keys.contains(groupName)) {
+                    GroupConfigTable.GroupConfigItems groupConfigItems = new GroupConfigTable.GroupConfigItems(
+                        groupName);
+                    groupConfigTable.addGroupConfigItems(groupConfigItems);
+                    rspGroupConfigItems.getItemsList().forEach(item -> {
+                        groupConfigItems.add(new ConfigTable.ConfigItem(item.getName(), item.getValue()));
+                    });
+                }
+            });
+            this.groupUuid = responseUuid;
+        } catch (Exception e) {
+            log.error("Remote config center [{}}] is not available.", settings, e);

Review comment:
       ```suggestion
               log.error("Remote config center [{}] is not available.", settings, e);
   ```

##########
File path: oap-server/server-configuration/grpc-configuration-sync/src/test/java/org/apache/skywalking/oap/server/configuration/grpc/GRPCConfigurationTest.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.skywalking.oap.server.configuration.grpc;
+
+import io.grpc.testing.GrpcServerRule;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.configuration.api.ConfigChangeWatcher;
+import org.apache.skywalking.oap.server.configuration.api.GroupConfigChangeWatcher;
+import org.apache.skywalking.oap.server.configuration.service.ConfigurationServiceGrpc;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.powermock.reflect.Whitebox;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+
+@Slf4j
+public class GRPCConfigurationTest {
+    private GRPCConfigurationProvider provider;
+    private GRPCConfigWatcherRegister register;
+    private ConfigChangeWatcher singleWatcher;
+    private GroupConfigChangeWatcher groupWatcher;
+
+    @Rule
+    public final GrpcServerRule grpcServerRule = new GrpcServerRule().directExecutor();
+
+    @Before
+    public void before() {
+        //for create register
+        RemoteEndpointSettings settings = new RemoteEndpointSettings();
+        settings.setHost("localhost");
+        settings.setPort(5678);
+        settings.setPeriod(1);
+        provider = new GRPCConfigurationProvider();
+        register = new GRPCConfigWatcherRegister(settings);
+        ConfigurationServiceGrpc.ConfigurationServiceBlockingStub blockingStub = ConfigurationServiceGrpc.newBlockingStub(
+            grpcServerRule.getChannel());
+        Whitebox.setInternalState(register, "stub", blockingStub);
+        initWatcher();
+        assertNotNull(provider);
+    }
+
+    @Test(timeout = 20000)
+    public void shouldReadUpdated() throws Exception {
+        AtomicInteger dataFlage = new AtomicInteger(0);
+        grpcServerRule.getServiceRegistry().addService(new MockGRPCConfigService(dataFlage));
+        assertNull(singleWatcher.value());
+        register.registerConfigChangeWatcher(singleWatcher);
+        register.start();
+
+        for (String v = singleWatcher.value(); v == null; v = singleWatcher.value()) {
+        }
+        assertEquals("100", singleWatcher.value());
+        //change
+        dataFlage.set(1);
+        TimeUnit.SECONDS.sleep(1);
+        for (String v = singleWatcher.value(); v.equals("100"); v = singleWatcher.value()) {
+        }
+        assertEquals("300", singleWatcher.value());
+        //no change
+        dataFlage.set(2);
+        TimeUnit.SECONDS.sleep(3);
+        for (String v = singleWatcher.value(); !v.equals("300"); v = singleWatcher.value()) {
+        }
+        assertEquals("300", singleWatcher.value());
+        //delete
+        dataFlage.set(3);
+        TimeUnit.SECONDS.sleep(1);
+        for (String v = singleWatcher.value(); v.equals("300"); v = singleWatcher.value()) {
+        }
+        assertEquals("", singleWatcher.value());
+    }
+
+    @Test(timeout = 20000)
+    public void shouldReadUpdated4Group() throws Exception {
+        AtomicInteger dataFlage = new AtomicInteger(0);

Review comment:
       ```suggestion
           AtomicInteger dataFlag = new AtomicInteger(0);
   ```

##########
File path: oap-server/server-configuration/grpc-configuration-sync/src/test/java/org/apache/skywalking/oap/server/configuration/grpc/GRPCConfigurationTest.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.skywalking.oap.server.configuration.grpc;
+
+import io.grpc.testing.GrpcServerRule;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.configuration.api.ConfigChangeWatcher;
+import org.apache.skywalking.oap.server.configuration.api.GroupConfigChangeWatcher;
+import org.apache.skywalking.oap.server.configuration.service.ConfigurationServiceGrpc;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.powermock.reflect.Whitebox;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+
+@Slf4j
+public class GRPCConfigurationTest {
+    private GRPCConfigurationProvider provider;
+    private GRPCConfigWatcherRegister register;
+    private ConfigChangeWatcher singleWatcher;
+    private GroupConfigChangeWatcher groupWatcher;
+
+    @Rule
+    public final GrpcServerRule grpcServerRule = new GrpcServerRule().directExecutor();
+
+    @Before
+    public void before() {
+        //for create register
+        RemoteEndpointSettings settings = new RemoteEndpointSettings();
+        settings.setHost("localhost");
+        settings.setPort(5678);
+        settings.setPeriod(1);
+        provider = new GRPCConfigurationProvider();
+        register = new GRPCConfigWatcherRegister(settings);
+        ConfigurationServiceGrpc.ConfigurationServiceBlockingStub blockingStub = ConfigurationServiceGrpc.newBlockingStub(
+            grpcServerRule.getChannel());
+        Whitebox.setInternalState(register, "stub", blockingStub);
+        initWatcher();
+        assertNotNull(provider);
+    }
+
+    @Test(timeout = 20000)
+    public void shouldReadUpdated() throws Exception {
+        AtomicInteger dataFlage = new AtomicInteger(0);

Review comment:
       ```suggestion
           AtomicInteger dataFlag = new AtomicInteger(0);
   ```




-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #7709: Support gRPC sync grouped dynamic configurations.

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7709:
URL: https://github.com/apache/skywalking/pull/7709#discussion_r707934012



##########
File path: oap-server/server-configuration/grpc-configuration-sync/src/main/test/java/org/apache/skywalking/oap/server/configuration/grpc/GRPCConfigurationTest.java
##########
@@ -0,0 +1,177 @@
+/*
+ * 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.skywalking.oap.server.configuration.grpc;
+
+import io.grpc.testing.GrpcServerRule;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.configuration.api.ConfigChangeWatcher;
+import org.apache.skywalking.oap.server.configuration.api.GroupConfigChangeWatcher;
+import org.apache.skywalking.oap.server.configuration.service.ConfigurationServiceGrpc;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.powermock.reflect.Whitebox;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+
+@Slf4j
+public class GRPCConfigurationTest {
+    private GRPCConfigurationProvider provider;
+    private GRPCConfigWatcherRegister register;
+    private ConfigChangeWatcher singleWatcher;
+    private GroupConfigChangeWatcher groupWatcher;
+    /**
+     * 0:init, 1:change, 2:no change, 3:delete
+     */
+    public static volatile int singleDataFlag;
+    /**
+     * 0:init, 1:change, 2:no change, 3:delete
+     */
+    public static volatile int groupDataFlag;

Review comment:
       Using a static variable to pass states between classes is not a good practice, although it's only a test, please consider other mechanism, e.g. passing an `AtomicInteger` as a constructor parameter of `MockGRPCConfigService` class




-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [skywalking] wu-sheng commented on pull request #7709: Support gRPC sync grouped dynamic configurations.

Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #7709:
URL: https://github.com/apache/skywalking/pull/7709#issuecomment-918858842


   Please fix the UT.


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #7709: Support gRPC sync grouped dynamic configurations.

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7709:
URL: https://github.com/apache/skywalking/pull/7709#discussion_r707931963



##########
File path: oap-server/server-configuration/grpc-configuration-sync/src/main/java/org/apache/skywalking/oap/server/configuration/grpc/GRPCConfigWatcherRegister.java
##########
@@ -70,14 +70,42 @@ public GRPCConfigWatcherRegister(RemoteEndpointSettings settings) {
             });
             this.uuid = responseUuid;
         } catch (Exception e) {
-            LOGGER.error("Remote config center [" + settings + "] is not available.", e);
+            log.error("Remote config center [" + settings + "] is not available.", e);
         }
         return Optional.of(table);
     }
 
     @Override
     public Optional<GroupConfigTable> readGroupConfig(final Set<String> keys) {
-        // TODO: implement readGroupConfig
-        return Optional.empty();
+        GroupConfigTable groupConfigTable = new GroupConfigTable();
+        try {
+            ConfigurationRequest.Builder builder = ConfigurationRequest.newBuilder()
+                                                                       .setClusterName(settings.getClusterName());
+            if (groupUuid != null) {
+                builder.setUuid(groupUuid);
+            }
+            GroupConfigurationResponse response = stub.callGroup(builder.build());
+            String responseUuid = response.getUuid();
+            if (Objects.equals(groupUuid, responseUuid)) {
+                // If UUID matched, the config table is expected as empty.
+                return Optional.empty();
+            }
+
+            response.getGroupConfigTableList().forEach(rspGroupConfigItems -> {
+                String groupName = rspGroupConfigItems.getGroupName();
+                if (keys.contains(groupName)) {
+                    GroupConfigTable.GroupConfigItems groupConfigItems = new GroupConfigTable.GroupConfigItems(
+                        groupName);
+                    groupConfigTable.addGroupConfigItems(groupConfigItems);
+                    rspGroupConfigItems.getItemsList().forEach(item -> {
+                        groupConfigItems.add(new ConfigTable.ConfigItem(item.getName(), item.getValue()));
+                    });
+                }
+            });
+            this.groupUuid = responseUuid;
+        } catch (Exception e) {
+            log.error("Remote config center [" + settings + "] is not available.", e);

Review comment:
       Do not use string concatenation, use `{}` 




-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [skywalking] kezhenxu94 edited a comment on pull request #7709: Support gRPC sync grouped dynamic configurations.

Posted by GitBox <gi...@apache.org>.
kezhenxu94 edited a comment on pull request #7709:
URL: https://github.com/apache/skywalking/pull/7709#issuecomment-918821538


   Please format the new codes, some lines are very


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [skywalking] kezhenxu94 commented on pull request #7709: Support gRPC sync grouped dynamic configurations.

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #7709:
URL: https://github.com/apache/skywalking/pull/7709#issuecomment-918821538


   Please format the new files


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [skywalking] wu-sheng merged pull request #7709: Support gRPC sync grouped dynamic configurations.

Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #7709:
URL: https://github.com/apache/skywalking/pull/7709


   


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #7709: Support gRPC sync grouped dynamic configurations.

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7709:
URL: https://github.com/apache/skywalking/pull/7709#discussion_r707936250



##########
File path: oap-server/server-configuration/grpc-configuration-sync/src/main/test/java/org/apache/skywalking/oap/server/configuration/grpc/GRPCConfigurationTest.java
##########
@@ -0,0 +1,188 @@
+/*
+ * 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.skywalking.oap.server.configuration.grpc;
+
+import io.grpc.testing.GrpcServerRule;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.TimeUnit;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.skywalking.oap.server.configuration.api.ConfigChangeWatcher;
+import org.apache.skywalking.oap.server.configuration.api.GroupConfigChangeWatcher;
+import org.apache.skywalking.oap.server.configuration.service.ConfigurationServiceGrpc;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.powermock.reflect.Whitebox;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+
+@Slf4j
+public class GRPCConfigurationTest {
+    private GRPCConfigurationProvider provider;
+    private GRPCConfigWatcherRegister register;
+    private ConfigChangeWatcher singleWatcher;
+    private GroupConfigChangeWatcher groupWatcher;
+    /**
+     * 0:init, 1:change, 2:no change, 3:delete
+     */
+    public static volatile int singleDataFlag;
+    /**
+     * 0:init, 1:change, 2:no change, 3:delete
+     */
+    public static volatile int groupDataFlag;
+    @Rule
+    public final GrpcServerRule grpcServerRule = new GrpcServerRule().directExecutor();
+
+    @Before
+    public void before() {
+        //for create register
+        RemoteEndpointSettings settings = new RemoteEndpointSettings();
+        settings.setHost("localhost");
+        settings.setPort(5678);
+        settings.setPeriod(1);
+        provider = new GRPCConfigurationProvider();
+        register = new GRPCConfigWatcherRegister(settings);
+        grpcServerRule.getServiceRegistry().addService(new MockGRPCConfigService());
+        ConfigurationServiceGrpc.ConfigurationServiceBlockingStub blockingStub = ConfigurationServiceGrpc.newBlockingStub(
+            grpcServerRule.getChannel());
+        Whitebox.setInternalState(register, "stub", blockingStub);
+        initWatcher();
+        assertNotNull(provider);
+    }
+
+    @Test(timeout = 20000)
+    public void shouldReadUpdated() throws Exception {
+        assertNull(singleWatcher.value());
+        register.registerConfigChangeWatcher(singleWatcher);
+        register.start();
+        singleDataFlag = 0;
+        for (String v = singleWatcher.value(); v == null; v = singleWatcher.value()) {
+        }
+        assertEquals("100", singleWatcher.value());
+        //change
+        singleDataFlag = 1;
+        TimeUnit.SECONDS.sleep(1);
+        for (String v = singleWatcher.value(); v.equals("100"); v = singleWatcher.value()) {
+        }
+        assertEquals("300", singleWatcher.value());
+        //no change
+        singleDataFlag = 2;
+        TimeUnit.SECONDS.sleep(3);
+        for (String v = singleWatcher.value(); !v.equals("300"); v = singleWatcher.value()) {
+        }
+        assertEquals("300", singleWatcher.value());
+        //delete
+        singleDataFlag = 3;
+        TimeUnit.SECONDS.sleep(1);
+        for (String v = singleWatcher.value(); v.equals("300"); v = singleWatcher.value()) {
+        }
+        assertEquals("", singleWatcher.value());
+    }
+
+    @Test(timeout = 20000)
+    public void shouldReadUpdated4Group() throws Exception {
+        assertEquals("{}", groupWatcher.groupItems().toString());
+        register.registerConfigChangeWatcher(groupWatcher);
+        register.start();
+        groupDataFlag = 0;
+        for (String v = groupWatcher.groupItems().get("item1");
+            v == null;
+            v = groupWatcher.groupItems().get("item1")) {
+        }
+        assertEquals("100", groupWatcher.groupItems().get("item1"));
+        for (String v = groupWatcher.groupItems().get("item2");
+            v == null;
+            v = groupWatcher.groupItems().get("item2")) {
+        }
+        assertEquals("200", groupWatcher.groupItems().get("item2"));
+        //change item2
+        groupDataFlag = 1;
+        TimeUnit.SECONDS.sleep(1);
+        for (String v = groupWatcher.groupItems().get("item2");
+            v.equals("200");
+            v = groupWatcher.groupItems().get("item2")) {
+        }
+        assertEquals("2000", groupWatcher.groupItems().get("item2"));
+        //no change
+        groupDataFlag = 2;
+        TimeUnit.SECONDS.sleep(3);
+        for (String v = groupWatcher.groupItems().get("item1");
+            v.equals("100");
+            v = groupWatcher.groupItems().get("item2")) {
+        }

Review comment:
       ```suggestion
           for (String v = groupWatcher.groupItems().get("item1");
               v.equals("100");
               v = groupWatcher.groupItems().get("item1")) {
           }
   ```




-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #7709: Support gRPC sync grouped dynamic configurations.

Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #7709:
URL: https://github.com/apache/skywalking/pull/7709#discussion_r707930995



##########
File path: oap-server/server-configuration/grpc-configuration-sync/src/main/test/java/org/apache/skywalking/oap/server/configuration/grpc/GRPCConfigurationTest.java
##########
@@ -0,0 +1,177 @@
+/*

Review comment:
       Please check, the test resource directory structure is wrong, how do you test this locally?
   
   oap-server/server-configuration/grpc-configuration-sync/src/main/test/java/org/apache/skywalking/oap/server/configuration/grpc/GRPCConfigurationTest.java




-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [skywalking] kezhenxu94 edited a comment on pull request #7709: Support gRPC sync grouped dynamic configurations.

Posted by GitBox <gi...@apache.org>.
kezhenxu94 edited a comment on pull request #7709:
URL: https://github.com/apache/skywalking/pull/7709#issuecomment-918821538


   Please format the new codes, some lines are very long


-- 
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: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org