ICEpush
  1. ICEpush
  2. PUSH-26

Log a warning when pushing to a non-existent group

          Details

          • Type: Improvement Improvement
          • Status: Closed
          • Priority: Major Major
          • Resolution: Fixed
          • Affects Version/s: 2.0-Alpha2
          • Fix Version/s: 2.0-Alpha3
          • Component/s: Push Library
          • Labels:
            None
          • Environment:
            n/a

            Description

            It will be very useful in development to log a warning when PushContext.push(String group) is called for a non-existent group. Currently, the group name is treated as a single pushId if the group does not exist, but we could still check if that pushId exists, and if not, log a warning. This will improve the monitoring capabilities of an ICEpush application.

            Note that in a clustered environment a node may have no knowledge of a particular group, and a warning might be erroneously logged.

              Activity

              Ascending order - Click to sort in descending order
              Hide
              Permalink
              Mircea Toma added a comment - - edited

              Keep track of registered pushIDs to log warning when rogue pushIDs are used. To avoid extensive group scanning on each notification call a special Set implementation is used. Its implementation keeps track of the number of times a pushId is used to avoid removing it from the set of registered pushIds if multiple groups own the same pushId.

              Show
              Mircea Toma added a comment - - edited Keep track of registered pushIDs to log warning when rogue pushIDs are used. To avoid extensive group scanning on each notification call a special Set implementation is used. Its implementation keeps track of the number of times a pushId is used to avoid removing it from the set of registered pushIds if multiple groups own the same pushId.
              Hide
              Permalink
              Ted Goddard added a comment -

              This feature may have disappeared with the refactoring to include cluster support.

              The other concern, though, is that the logging may be unbounded. For instance, closing a window in auction results in:

              Mar 15, 2010 4:51:34 PM org.icepush.PushContext push
              WARNING: 'auction' pushId does not belong to a group.
              Mar 15, 2010 4:51:35 PM org.icepush.PushContext push
              WARNING: 'auction' pushId does not belong to a group.
              Mar 15, 2010 4:51:36 PM org.icepush.PushContext push
              WARNING: 'auction' pushId does not belong to a group.

              To avoid storing the name of every failed group, it might be sufficient to suppress the logging just for the last failed group.

              Show
              Ted Goddard added a comment - This feature may have disappeared with the refactoring to include cluster support. The other concern, though, is that the logging may be unbounded. For instance, closing a window in auction results in: Mar 15, 2010 4:51:34 PM org.icepush.PushContext push WARNING: 'auction' pushId does not belong to a group. Mar 15, 2010 4:51:35 PM org.icepush.PushContext push WARNING: 'auction' pushId does not belong to a group. Mar 15, 2010 4:51:36 PM org.icepush.PushContext push WARNING: 'auction' pushId does not belong to a group. To avoid storing the name of every failed group, it might be sufficient to suppress the logging just for the last failed group.
              Hide
              Permalink
              Philip Breau added a comment -

              Is that warning being logged in auction because of the older icepush.jar version currently used in Glimmer which was checking pushIDs directly (when you previous could push to id's directly)? Couldn't we just move the logging to LocalPushGroupManager like so. The warning could just be ignored for clustered deployments.

              Index: /home/philip/workspace/ICEpush/core/src/main/java/org/icepush/LocalPushGroupManager.java
              ===================================================================
              — /home/philip/workspace/ICEpush/core/src/main/java/org/icepush/LocalPushGroupManager.java (revision 20901)
              +++ /home/philip/workspace/ICEpush/core/src/main/java/org/icepush/LocalPushGroupManager.java (working copy)
              @@ -94,7 +94,10 @@
              if (LOGGER.isLoggable(Level.FINEST))

              { LOGGER.log(Level.FINEST, "Push notification triggered for '" + groupName + "' group."); }
              • outboundNotifier.notifyObservers(groupMap.get(groupName).getPushIDs());
                + if( ! groupMap.containsKey(groupName))
                + LOGGER.warning(..);
                + else
                + outboundNotifier.notifyObservers(groupMap.get(groupName).getPushIDs());
                }
                }
              Show
              Philip Breau added a comment - Is that warning being logged in auction because of the older icepush.jar version currently used in Glimmer which was checking pushIDs directly (when you previous could push to id's directly)? Couldn't we just move the logging to LocalPushGroupManager like so. The warning could just be ignored for clustered deployments. Index: /home/philip/workspace/ICEpush/core/src/main/java/org/icepush/LocalPushGroupManager.java =================================================================== — /home/philip/workspace/ICEpush/core/src/main/java/org/icepush/LocalPushGroupManager.java (revision 20901) +++ /home/philip/workspace/ICEpush/core/src/main/java/org/icepush/LocalPushGroupManager.java (working copy) @@ -94,7 +94,10 @@ if (LOGGER.isLoggable(Level.FINEST)) { LOGGER.log(Level.FINEST, "Push notification triggered for '" + groupName + "' group."); } outboundNotifier.notifyObservers(groupMap.get(groupName).getPushIDs()); + if( ! groupMap.containsKey(groupName)) + LOGGER.warning(..); + else + outboundNotifier.notifyObservers(groupMap.get(groupName).getPushIDs()); } }

                People

                • Assignee:
                  Mircea Toma
                  Reporter:
                  Philip Breau
                • Votes:
                  0 Vote for this issue
                  Watchers:
                  0 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved: