NotificationControllerV2#handleMessage may set outdated result

[PleaseGiveMeTheCoke]

At the end of the NotificationControllerV2#handleMessage method, the result of the deferredResults are set in the following way

    for (DeferredResultWrapper result : results) {
      result.setResult(configNotification);
    }

Is it possible that the notificationId in configNotification is less than the id on the client side, causing this to be an unnecessary result notification?

[dosubot[bot]]

🤖

Hey there, @PleaseGiveMeTheCoke! :wave: I'm Dosu, a friendly bot here to assist you with bugs, answer your queries, and guide you on your journey to become a contributor while we wait for a human maintainer. Let's dive into this together! :swimmer:

Yes, it is possible that the notificationId in configNotification could be less than the id on the client side, resulting in an unnecessary result notification in the NotificationControllerV2#handleMessage method. This could occur in the filterNotifications method, where the notificationId of the client side is compared with the notificationId of the server side. If the notificationId of the client side is greater, the server side notification is ignored. Here is the relevant code snippet:

private Map<String, ApolloConfigNotification> filterNotifications(String appId,
                                                                    List<ApolloConfigNotification> notifications) {
    Map<String, ApolloConfigNotification> filteredNotifications = Maps.newHashMap();
    for (ApolloConfigNotification notification : notifications) {
      if (Strings.isNullOrEmpty(notification.getNamespaceName())) {
        continue;
      }
      //strip out .properties suffix
      String originalNamespace = namespaceUtil.filterNamespaceName(notification.getNamespaceName());
      notification.setNamespaceName(originalNamespace);
      //fix the character case issue, such as FX.apollo <-> fx.apollo
      String normalizedNamespace = namespaceUtil.normalizeNamespace(appId, originalNamespace);

      // in case client side namespace name has character case issue and has difference notification ids
      // such as FX.apollo = 1 but fx.apollo = 2, we should let FX.apollo have the chance to update its notification id
      // which means we should record FX.apollo = 1 here and ignore fx.apollo = 2
      if (filteredNotifications.containsKey(normalizedNamespace) &&
          filteredNotifications.get(normalizedNamespace).getNotificationId() < notification.getNotificationId()) {
        continue;
      }

      filteredNotifications.put(normalizedNamespace, notification);
    }
    return filteredNotifications;
  }

You can find this code in the NotificationControllerV2.java file.

The notificationId is generated and updated on the server side through the ReleaseMessage entity. This entity is fetched from the releaseMessageService which interacts with the database to fetch the latest ReleaseMessage for the given watched keys. When a new configuration is published, a new ReleaseMessage is created with a new notificationId, which triggers the server to send a notification to the client with the updated notificationId. You can find more details in the NotificationController.java file.

I hope this answers your question. If you need further clarification, feel free to ask.

Best regards, Dosu

Sources

• apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/NotificationControllerV2.java
• apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/NotificationController.java

---

This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

[PleaseGiveMeTheCoke]

🤖

Hey there, @PleaseGiveMeTheCoke! 👋 I'm Dosu, a friendly bot here to assist you with bugs, answer your queries, and guide you on your journey to become a contributor while we wait for a human maintainer. Let's dive into this together! 🏊

Yes, it is possible that the notificationId in configNotification could be less than the id on the client side, resulting in an unnecessary result notification in the NotificationControllerV2#handleMessage method. This could occur in the filterNotifications method, where the notificationId of the client side is compared with the notificationId of the server side. If the notificationId of the client side is greater, the server side notification is ignored. Here is the relevant code snippet:

private Map<String, ApolloConfigNotification> filterNotifications(String appId,
                                                                    List<ApolloConfigNotification> notifications) {
    Map<String, ApolloConfigNotification> filteredNotifications = Maps.newHashMap();
    for (ApolloConfigNotification notification : notifications) {
      if (Strings.isNullOrEmpty(notification.getNamespaceName())) {
        continue;
      }
      //strip out .properties suffix
      String originalNamespace = namespaceUtil.filterNamespaceName(notification.getNamespaceName());
      notification.setNamespaceName(originalNamespace);
      //fix the character case issue, such as FX.apollo <-> fx.apollo
      String normalizedNamespace = namespaceUtil.normalizeNamespace(appId, originalNamespace);

      // in case client side namespace name has character case issue and has difference notification ids
      // such as FX.apollo = 1 but fx.apollo = 2, we should let FX.apollo have the chance to update its notification id
      // which means we should record FX.apollo = 1 here and ignore fx.apollo = 2
      if (filteredNotifications.containsKey(normalizedNamespace) &&
          filteredNotifications.get(normalizedNamespace).getNotificationId() < notification.getNotificationId()) {
        continue;
      }

      filteredNotifications.put(normalizedNamespace, notification);
    }
    return filteredNotifications;
  }

You can find this code in the NotificationControllerV2.java file.

The notificationId is generated and updated on the server side through the ReleaseMessage entity. This entity is fetched from the releaseMessageService which interacts with the database to fetch the latest ReleaseMessage for the given watched keys. When a new configuration is published, a new ReleaseMessage is created with a new notificationId, which triggers the server to send a notification to the client with the updated notificationId. You can find more details in the NotificationController.java file.

I hope this answers your question. If you need further clarification, feel free to ask.

Best regards, Dosu

Sources

• apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/NotificationControllerV2.java
• apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/NotificationController.java

This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

Dosu所说的这个关于NotificationId的比较似乎和我说的场景不一样。

1. Dosu提到的filterNotifications仅仅是为了过滤掉一些特殊场景，比如FX.apollo = 1 but fx.apollo = 2

2. 即使filterNotifications能过滤掉小于客户端的NotificationId，但filterNotifications发生在设置结果之前。如何保证设置结果的时候，也就是handleMessage方法里设置的NotificationId一定大于client端的？请帮我解决这个疑问 @nobodyiam

   This comparison Dosu's talking about regarding NotificationId doesn't seem to be the same scenario I'm talking about.

3. Dosu mentioned filterNotifications just to filter out some special scenarios like fx.apollo = 1 but fx.apollo = 2
4. even though filterNotifications can filter out NotificationId's that are smaller than the client's, the filterNotifications happen before the result is set. How to make sure that the NotificationId set in the handleMessage method must be greater than the client side when setting the result? Please help me with my query @nobodyiam

[dosubot[bot]]

🤖

The Apollo configuration management system ensures that the NotificationId set in the handleMessage method is always greater than the client side when setting the result by checking if the latest ReleaseMessage's id is not equal to the client's notificationId in the pollNotification method. If the latest is not null and its id is not equal to notificationId, it means there is a newer configuration available, so it sets the result immediately with the latest ReleaseMessage's id.

Here is the relevant code snippet:

ReleaseMessage latest = releaseMessageService.findLatestReleaseMessageForMessages(watchedKeys);

if (latest != null && latest.getId() != notificationId) {
  deferredResult.setResult(new ResponseEntity<>(
      new ApolloConfigNotification(namespace, latest.getId()), HttpStatus.OK));
}

If the latest is null or its id is equal to notificationId, it means the client's configuration is up-to-date. In this case, the client's request is registered and put into deferredResults for later processing. When a new ReleaseMessage comes (handled in handleMessage method), it will notify all the registered clients whose notificationId is less than the new ReleaseMessage's id.

Here is the relevant code snippet:

if (!deferredResults.containsKey(content)) {
  return;
}
List<DeferredResult<ResponseEntity<ApolloConfigNotification>>> results =
    Lists.newArrayList(deferredResults.get(content));
for (DeferredResult<ResponseEntity<ApolloConfigNotification>> result : results) {
  result.setResult(notification);
}

So, the filterNotifications method is not directly involved in ensuring the NotificationId is always greater on the server side. The filterNotifications method is not even present in the provided context.

The handleMessage method in the Apollo configuration management system is an interface method that is designed to be implemented by classes that listen for ReleaseMessage instances. When a ReleaseMessage is received, the handleMessage method is called with the ReleaseMessage and the channel it was received on as arguments. The specific actions taken in response to the ReleaseMessage will depend on the implementation of the handleMessage method in the listener class.

In the context of Apollo, a ReleaseMessage typically signifies that a configuration has been released or updated, so the handleMessage method would likely involve updating the local configuration to reflect the changes. However, the exact details would depend on the specific implementation.

You can find more details in the source code of Apollo:

• apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/NotificationController.java
• apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/message/ReleaseMessageListener.java

I hope this helps! If you have any more questions, feel free to ask.

Sources

• apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/controller/NotificationController.java
• apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/message/ReleaseMessageListener.java

---

This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

[nobodyiam]

even though filterNotifications can filter out NotificationId's that are smaller than the client's, the filterNotifications happen before the result is set. How to make sure that the NotificationId set in the handleMessage method must be greater than the client side when setting the result?

I don't think we need to make sure the NotificationId set in the handleMessage method must be greater than the client side. The notification is only an event telling the client that some new configuration released. Even though the id is smaller than the client side, that only means the client already fetched the latest configurations. And we had the notification id check logic in ConfigController.

[PleaseGiveMeTheCoke]

I don't think we need to make sure the NotificationId set in the handleMessage method must be greater than the client side. The notification is only an event telling the client that some new configuration released. Even though the id is smaller than the client side, that only means the client already fetched the latest configurations. And we had the notification id check logic in ConfigController.

Thanks for responding to my query, I have found the checksum logic for the notificationId in the ConfigController as you said, which does ensure that the client gets the latest configuration every time it fetches it. But I still have a question, would it not be better to add the validation in handleMessage? This can avoid notificationId outdated result setting and reduce the rpc loss for client to do a configuration query. There may be something I'm not thinking about, such as adding a checksum to the notificationId in the handleMessage would have other negative impacts, and if it does I'd be grateful for an answer!

[nobodyiam]

would it not be better to add the validation in handleMessage

It could be done. However, I suspect the chances that the server-side notification ID is less than the client-side is very rare, so it's not that necessary.

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically closed because it has not had activity in the last 7 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.