[PUSH-26] Log a warning when pushing to a non-existent group - ICEsoft JIRA Issue Tracker

Details

Type: Improvement
Status: Closed
Priority: 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

Descending order - Click to sort in ascending order

Hide

Permalink

Philip Breau added a comment - 15/Mar/10 6:20 PM

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 - 15/Mar/10 6:20 PM 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()); } }

Hide

Permalink

Ted Goddard added a comment - 15/Mar/10 5:59 PM

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 - 15/Mar/10 5:59 PM 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

Mircea Toma added a comment - 22/Feb/10 5:26 PM - 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 - 22/Feb/10 5:26 PM - 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.

Log a warning when pushing to a non-existent group

Details

Description

Activity

People

Dates