Skip to content

Commit

Permalink
SAK-31760 Make sure we never have null last visit time. (sakaiproject…
Browse files Browse the repository at this point in the history 
…#4515)

* SAK-31760 Small cleanup and JavaDoc.

Also introduces a test that shows the problem of presence begin and end events in the same update.

* SAK-31760 Make sure last visit is never null.

We do this by using the date of the SitePresence rather than the start time.
  • Loading branch information
@buckett
buckett authored Jun 29, 2017
1 parent f36af5a commit 8d2d157
Show file tree
Hide file tree
Showing 6 changed files with 79 additions and 60 deletions.
4 changes: 3 additions & 1 deletion sitestats/sitestats-api/src/java/org/sakaiproject/sitestats/api/SitePresence.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ public interface SitePresence extends Stat, Comparable<SitePresence> {
/** Set time spent (in milliseconds) */
public void setDuration(long duration);

/** Get (temporary) last visit start time */
/**
* Get (temporary) last visit start time. This is used when the begin event happens in one batch update and
* the end update happens in another batch update and so we need to persist the start time. */
public Date getLastVisitStartTime();

/** Set (temporary) last visit start time */
Expand Down
6 changes: 6 additions & 0 deletions sitestats/sitestats-api/src/java/org/sakaiproject/sitestats/api/SitePresenceTotal.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ public interface SitePresenceTotal {

public void incrementTotalVisits();

/**
* This increments the visits and updates the last visit time from the passed presence.
* @param sp The SitePresence to update from.
*/
public void updateFrom(SitePresence sp);

public long getId();
public void setId(long id);
public String getUserId();
Expand Down
11 changes: 10 additions & 1 deletion ...ts/sitestats-impl-hib/src/java/org/sakaiproject/sitestats/impl/SitePresenceTotalImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@

@Getter
@Setter
/**
* Like the SitePresence but it includes a total number of visits and when they last visited for a site/user.
* @see SitePresence
*/
public class SitePresenceTotalImpl implements SitePresenceTotal, Serializable {

private static final long serialVersionUID = 1L;
Expand All @@ -46,7 +50,12 @@ public SitePresenceTotalImpl(SitePresence sp) {
siteId = sp.getSiteId();
userId = sp.getUserId();
totalVisits = 1;
setLastVisitTime(sp.getLastVisitStartTime());
setLastVisitTime(sp.getDate());
}

public void updateFrom(SitePresence sp) {
incrementTotalVisits();
setLastVisitTime(sp.getDate());
}

public void incrementTotalVisits() {
Expand Down
4 changes: 2 additions & 2 deletions ...stats-impl-hib/src/java/org/sakaiproject/sitestats/impl/hbm/SitePresenceTotalImpl.hbm.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
<property name="totalVisits" type="integer" not-null="true">
<column name="TOTAL_VISITS" not-null="true" />
</property>
<property name="lastVisitTime" type="timestamp">
<column name="LAST_VISIT_TIME" />
<property name="lastVisitTime" type="timestamp" not-null="true">
<column name="LAST_VISIT_TIME" not-null="true"/>
</property>
</class>
</hibernate-mapping>
83 changes: 27 additions & 56 deletions ...stats/sitestats-impl/src/java/org/sakaiproject/sitestats/impl/StatsUpdateManagerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,13 @@ private void preProcessEvent(Event event) {

//else LOG.debug("EventInfo ignored: '"+e.toString()+"' ("+e.toString()+") USER_ID: "+userId);
}


/**
* This processes events and updates in-memory cache. This doesn't push those changes back to the
* DB yet.
*
* @param dateTime Can this be <code>null</code>?
*/
private void consolidateEvent(Date dateTime, String eventId, String resourceRef, String userId, String siteId) {
if(eventId == null)
return;
Expand Down Expand Up @@ -1251,53 +1257,7 @@ private void doUpdateSiteVisitsObjects(Session session, Collection<SiteVisits> o
session.saveOrUpdate(eExisting);
}
}

private void doUpdateSiteVisitTimeObjects(Session session, Collection<SitePresence> o) {
if(o == null) return;
List<SitePresence> objects = new ArrayList<SitePresence>(o);
Collections.sort(objects);
Iterator<SitePresence> i = objects.iterator();
while(i.hasNext()){
SitePresence eUpdate = i.next();
SitePresence eExisting = null;
String eExistingSiteId = null;
try{
Criteria c = session.createCriteria(SitePresenceImpl.class);
c.add(Expression.eq("siteId", eUpdate.getSiteId()));
c.add(Expression.eq("userId", eUpdate.getUserId()));
c.add(Expression.eq("date", eUpdate.getDate()));
try{
eExisting = (SitePresence) c.uniqueResult();
}catch(HibernateException ex){
try{
List<SitePresence> events = (List<SitePresence>) c.list();
if ((events!=null) && (events.size()>0)){
LOG.debug("More than 1 result when unique result expected.", ex);
eExisting = (SitePresence) c.list().get(0);
}else{
LOG.debug("No result found", ex);
eExisting = null;
}
}catch(Exception ex3){
eExisting = null;
}
}catch(Exception ex2){
LOG.warn("Probably ddbb error when loading data at java object", ex2);
}
if(eExisting == null){
eExisting = eUpdate;
}else{
eExisting.setDuration(eExisting.getDuration() + eUpdate.getDuration());
}
eExistingSiteId = eExisting.getSiteId();
}catch(Exception ex){
LOG.warn("Failed to event:" + eUpdate.getId(), ex);
}
if ((eExistingSiteId!=null) && (eExistingSiteId.trim().length()>0))
session.saveOrUpdate(eExisting);
}
}


private void doUpdateServerStatObjects(Session session, Collection<ServerStat> o) {
if(o == null) return;
List<ServerStat> objects = new ArrayList<ServerStat>(o);
Expand Down Expand Up @@ -1432,7 +1392,7 @@ private Map<UniqueVisitsKey, Integer> doGetSiteUniqueVisits(Session session, Map

private void doUpdateSitePresencesObjects(Session session, Collection<SitePresenceConsolidation> o) {
if(o == null) return;
List<SitePresenceConsolidation> objects = new ArrayList<SitePresenceConsolidation>(o);
List<SitePresenceConsolidation> objects = new ArrayList<>(o);
Collections.sort(objects);
Iterator<SitePresenceConsolidation> i = objects.iterator();
while(i.hasNext()){
Expand All @@ -1441,14 +1401,18 @@ private void doUpdateSitePresencesObjects(Session session, Collection<SitePresen
SitePresence sp = spc.sitePresence;
SitePresence spExisting = doGetSitePresence(session, sp.getSiteId(), sp.getUserId(), sp.getDate());
if(spExisting == null) {
// Should we be doing this save if this is an end event?
session.save(sp);
if (!spc.firstEventIsPresEnd) {
doUpdateSitePresenceTotal(session, sp);
} else {
// TODO Should deal with midnight crossing.
// Should we at least warn that we've just got a end event without a start.
}
}else{
long previousTotalPresence = spExisting.getDuration();
long previousPresence = 0;
long newTotalPresence = 0;
long newTotalPresence;
if(spc.firstEventIsPresEnd) {
if(spExisting.getLastVisitStartTime() != null)
previousPresence = spc.firstPresEndDate.getTime() - spExisting.getLastVisitStartTime().getTime();
Expand All @@ -1472,14 +1436,12 @@ private void doUpdateSitePresencesObjects(Session session, Collection<SitePresen
}

private void doUpdateSitePresenceTotal(Session session, SitePresence sp) throws Exception {

SitePresenceTotal spt = new SitePresenceTotalImpl(sp);
SitePresenceTotal sptExisting = doGetSitePresenceTotal(session, sp.getSiteId(), sp.getUserId());
if (sptExisting == null) {
SitePresenceTotal spt = new SitePresenceTotalImpl(sp);
session.save(spt);
} else {
sptExisting.incrementTotalVisits();
sptExisting.setLastVisitTime(sp.getLastVisitStartTime());
sptExisting.updateFrom(sp);
session.update(sptExisting);
}
}
Expand Down Expand Up @@ -1758,16 +1720,25 @@ private Date resetToDay(Date date){
return c.getTime();
}
}


/**
* This is used to hold the summary of a set of SitePresence objects in memory before
* they get written out to the DB later.
*/
private static class SitePresenceConsolidation implements Comparable<SitePresenceConsolidation>{
// Used to track if we never saw the presence start.
public boolean firstEventIsPresEnd;
public Date firstPresEndDate;
public SitePresence sitePresence;

public SitePresenceConsolidation(SitePresence sitePresence) {
this(sitePresence, null);
}


/**
* @param firstPresEndDate The date of the end of the first site presence and we never saw start. This may happen
* when we process the start in one batch and end in another batch.
*/
public SitePresenceConsolidation(SitePresence sitePresence, Date firstPresEndDate) {
this.sitePresence = sitePresence;
if(firstPresEndDate == null) {
Expand Down
31 changes: 31 additions & 0 deletions ...stats/sitestats-impl/src/test/org/sakaiproject/sitestats/test/StatsUpdateManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,37 @@ public void testSiteVisits() {
r4 = (List<ResourceStat>) db.getResultsForClass(ResourceStatImpl.class);
Assert.assertEquals(0, r4.size());
}

@Test
public void testSitePresenceSplitUpdates() {
// Start and end across collections.
{
List<Event> events = new ArrayList<>();
events.add(M_sum.buildEvent(new Date(), StatsManager.SITEVISIT_EVENTID, "/presence/" + FakeData.SITE_A_ID + "-presence", null, FakeData.USER_A_ID, "session-id"));
Assert.assertTrue(M_sum.collectEvents(events));
}
{
List<Event> events = new ArrayList<>();
events.add(M_sum.buildEvent(new Date(), StatsManager.SITEVISITEND_EVENTID, "/presence/" + FakeData.SITE_A_ID + "-presence", null, FakeData.USER_A_ID, "session-id"));
Assert.assertTrue(M_sum.collectEvents(events));
}

// Start and end in the same collection.
{
List<Event> events = new ArrayList<>();
events.add(M_sum.buildEvent(new Date(), StatsManager.SITEVISIT_EVENTID, "/presence/" + FakeData.SITE_A_ID + "-presence", null, FakeData.USER_A_ID, "session-id"));
events.add(M_sum.buildEvent(new Date(), StatsManager.SITEVISITEND_EVENTID, "/presence/" + FakeData.SITE_A_ID + "-presence", null, FakeData.USER_A_ID, "session-id"));
Assert.assertTrue(M_sum.collectEvents(events));
}
// Multiple end events in the same collection.
{
List<Event> events = new ArrayList<>();
events.add(M_sum.buildEvent(new Date(), StatsManager.SITEVISIT_EVENTID, "/presence/" + FakeData.SITE_A_ID + "-presence", null, FakeData.USER_A_ID, "session-id"));
events.add(M_sum.buildEvent(new Date(), StatsManager.SITEVISITEND_EVENTID, "/presence/" + FakeData.SITE_A_ID + "-presence", null, FakeData.USER_A_ID, "session-id"));
events.add(M_sum.buildEvent(new Date(), StatsManager.SITEVISITEND_EVENTID, "/presence/" + FakeData.SITE_A_ID + "-presence", null, FakeData.USER_A_ID, "session-id"));
Assert.assertTrue(M_sum.collectEvents(events));
}
}

// Activity tests
@SuppressWarnings("unchecked")
Expand Down

0 comments on commit 8d2d157

Please sign in to comment.