Skip to content

Commit

Permalink
changes based on review:
Browse files Browse the repository at this point in the history
1. use guava toStringHelper
2. use EnumMap
  • Loading branch information
wyan0220 committed Jul 10, 2012
1 parent 3cf8ebd commit 5577c22
Show file tree
Hide file tree
Showing 8 changed files with 111 additions and 115 deletions.
Expand Up @@ -15,6 +15,7 @@
*/
package com.proofpoint.discovery;

import com.google.common.base.Objects;
import com.google.common.collect.ImmutableSet;
import org.codehaus.jackson.annotate.JsonCreator;
import org.codehaus.jackson.annotate.JsonProperty;
Expand Down Expand Up @@ -150,7 +151,11 @@ public DynamicAnnouncement build()
@Override
public String toString()
{
return String.format("DynamicAnnouncement{environment:%s,location:%s,pool:%s,services:%s}", environment,
location, pool, services);
return Objects.toStringHelper(this.getClass())
.add("environment", environment)
.add("pool", pool)
.add("location", location)
.add("services", services)
.toString();
}
}
Expand Up @@ -15,6 +15,7 @@
*/
package com.proofpoint.discovery;

import com.google.common.base.Objects;
import com.google.common.collect.ImmutableMap;
import org.codehaus.jackson.annotate.JsonCreator;
import org.codehaus.jackson.annotate.JsonProperty;
Expand Down Expand Up @@ -125,7 +126,12 @@ public int hashCode()
@Override
public String toString()
{
return String.format("StaticAnnouncement{environment:%s,type:%s,pool:%s,location:%s,properties:%s}", environment, type,
pool, location, properties);
return Objects.toStringHelper(this.getClass())
.add("environment", environment)
.add("pool", pool)
.add("location", location)
.add("type", type)
.add("properties", properties)
.toString();
}
}
@@ -1,5 +1,6 @@
package com.proofpoint.discovery.monitor;

import com.google.common.base.Objects;
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import com.proofpoint.event.client.EventField;
Expand Down Expand Up @@ -115,7 +116,12 @@ public int hashCode()
@Override
public String toString()
{
return String.format("DiscoveryEvent{type:%s,success:%s,remoteAddress:%s,requestUri:%s,requestBodyJson:%s,processingDuration:%s}",
type, success, remoteAddress, requestUri, (requestBodyJson == null) ? "" : requestBodyJson, processingDuration);
return Objects.toStringHelper(this.getClass())
.add("type", type)
.add("success", success)
.add("remoteAddress", remoteAddress)
.add("requestUri", requestUri)
.add("requestBodyJson", requestBodyJson == null ? "" : requestBodyJson)

This comment has been minimized.

Copy link
@electrum

electrum Jul 11, 2012

This check doesn't seem necessary: it's fine to pass null to toStringHelper. If you do want it to be displayed as an empty string rather than null, then use Strings.nullToEmpty.

.add("processingDuration", processingDuration).toString();
}
}
Expand Up @@ -3,7 +3,6 @@
import com.google.common.base.Preconditions;
import com.google.inject.Inject;
import com.proofpoint.event.client.EventClient;
import com.proofpoint.log.Logger;
import com.proofpoint.units.Duration;

import java.util.concurrent.TimeUnit;
Expand All @@ -12,7 +11,6 @@ public class DiscoveryMonitor
{
private final EventClient eventClient;
private final DiscoveryStats stats;
private final Logger log = Logger.get(DiscoveryMonitor.class);

@Inject
public DiscoveryMonitor(EventClient eventClient, DiscoveryStats stats)
Expand All @@ -31,6 +29,5 @@ public void monitorDiscoveryEvent(DiscoveryEventType type, boolean success, Stri
public void monitorDiscoveryFailureEvent(DiscoveryEventType type, Exception e, String requestUri)
{
eventClient.post(new DiscoveryFailureEvent(type, e, requestUri));
log.warn(e, String.format("discovery request [%s] failed", requestUri));
}
}
Expand Up @@ -5,201 +5,182 @@
import org.weakref.jmx.Managed;
import org.weakref.jmx.Nested;

import java.util.EnumMap;
import java.util.concurrent.atomic.AtomicLong;

public class DiscoveryStats
{
private final AtomicLong serviceQuerySuccessCount = new AtomicLong();
private final AtomicLong serviceQueryFailureCount = new AtomicLong();
private final AtomicLong staticAnnouncementSuccessCount = new AtomicLong();
private final AtomicLong staticAnnouncementFailureCount = new AtomicLong();
private final AtomicLong staticAnnouncementListSuccessCount = new AtomicLong();
private final AtomicLong staticAnnouncementListFailureCount = new AtomicLong();
private final AtomicLong staticAnnouncementDeleteSuccessCount = new AtomicLong();
private final AtomicLong staticAnnouncementDeleteFailureCount = new AtomicLong();
private final AtomicLong dynamicAnnouncementSuccessCount = new AtomicLong();
private final AtomicLong dynamicAnnouncementFailureCount = new AtomicLong();
private final AtomicLong dynamicAnnouncementDeleteSuccessCount = new AtomicLong();
private final AtomicLong dynamicAnnouncementDeleteFailureCount = new AtomicLong();

private final TimedStat serviceQueryProcessingTime = new TimedStat();
private final TimedStat staticAnnouncementProcessingTime = new TimedStat();
private final TimedStat staticAnnouncementListProcessingTime = new TimedStat();
private final TimedStat staticAnnouncementDeleteProcessingTime = new TimedStat();
private final TimedStat dynamicAnnouncementProcessingTime = new TimedStat();
private final TimedStat dynamicAnnouncementDeleteProcessingTime = new TimedStat();
private final EnumMap<DiscoveryEventType, Stats> eventTypeStatsEnumMap;

This comment has been minimized.

Copy link
@electrum

electrum Jul 11, 2012

It's cleaner to call this eventTypeStats and to make the type Map rather than EnumMap. Always prefer to use the most generic collection class when declaring variables (e.g. List vs ArrayList or Map vs HashMap).


public DiscoveryStats()
{
eventTypeStatsEnumMap = new EnumMap<DiscoveryEventType, Stats>(DiscoveryEventType.class);
for (DiscoveryEventType type : DiscoveryEventType.values()) {
eventTypeStatsEnumMap.put(type, new Stats());
}
}

@Managed
public long getServiceQuerySuccessCount()
{
return serviceQuerySuccessCount.get();
return eventTypeStatsEnumMap.get(DiscoveryEventType.SERVICEQUERY).getSuccessCount();

This comment has been minimized.

Copy link
@electrum

electrum Jul 11, 2012

It can make the code a little shorter and easier to read if you static import the enum values.

}

@Managed
public long getServiceQueryFailureCount()
{
return serviceQueryFailureCount.get();
return eventTypeStatsEnumMap.get(DiscoveryEventType.SERVICEQUERY).getFailureCount();
}

@Managed
public long getStaticAnnouncementSuccessCount()
{
return staticAnnouncementSuccessCount.get();
return eventTypeStatsEnumMap.get(DiscoveryEventType.STATICANNOUNCEMENT).getSuccessCount();
}

@Managed
public long getStaticAnnouncementFailureCount()
{
return staticAnnouncementFailureCount.get();
return eventTypeStatsEnumMap.get(DiscoveryEventType.STATICANNOUNCEMENT).getFailureCount();
}

@Managed
public long getStaticAnnouncementListSuccessCount()
{
return staticAnnouncementListSuccessCount.get();
return eventTypeStatsEnumMap.get(DiscoveryEventType.STATICANNOUNCEMENTLIST).getSuccessCount();
}

@Managed
public long getStaticAnnouncementListFailureCount()
{
return staticAnnouncementListFailureCount.get();
return eventTypeStatsEnumMap.get(DiscoveryEventType.STATICANNOUNCEMENTLIST).getFailureCount();
}

@Managed
public long getStaticAnnouncementDeleteSuccessCount()
{
return staticAnnouncementDeleteSuccessCount.get();
return eventTypeStatsEnumMap.get(DiscoveryEventType.STATICANNOUNCEMENTDELETE).getSuccessCount();
}

@Managed
public long getStaticAnnouncementDeleteFailureCount()
{
return staticAnnouncementDeleteFailureCount.get();
return eventTypeStatsEnumMap.get(DiscoveryEventType.STATICANNOUNCEMENTDELETE).getFailureCount();
}

@Managed
public long getDynamicAnnouncementSuccessCount()
{
return dynamicAnnouncementSuccessCount.get();
return eventTypeStatsEnumMap.get(DiscoveryEventType.DYNAMICANNOUNCEMENT).getSuccessCount();
}

@Managed
public long getDynamicAnnouncementFailureCount()
{
return dynamicAnnouncementFailureCount.get();
return eventTypeStatsEnumMap.get(DiscoveryEventType.DYNAMICANNOUNCEMENT).getFailureCount();
}

@Managed
public long getDynamicAnnouncementDeleteSuccessCount()
{
return dynamicAnnouncementDeleteSuccessCount.get();
return eventTypeStatsEnumMap.get(DiscoveryEventType.DYNAMICANNOUNCEMENTDELETE).getSuccessCount();
}

@Managed
public long getDynamicAnnouncementDeleteFailureCount()
{
return dynamicAnnouncementDeleteFailureCount.get();
return eventTypeStatsEnumMap.get(DiscoveryEventType.DYNAMICANNOUNCEMENTDELETE).getFailureCount();
}

@Managed
@Nested
public TimedStat getServiceQueryProcessingTime()
{
return serviceQueryProcessingTime;
return eventTypeStatsEnumMap.get(DiscoveryEventType.SERVICEQUERY).getProcessingTime();
}

@Managed
@Nested
public TimedStat getStaticAnnouncementProcessingTime()
{
return staticAnnouncementProcessingTime;
return eventTypeStatsEnumMap.get(DiscoveryEventType.STATICANNOUNCEMENT).getProcessingTime();
}

@Managed
@Nested
public TimedStat getStaticAnnouncementListProcessingTime()
{
return staticAnnouncementListProcessingTime;
return eventTypeStatsEnumMap.get(DiscoveryEventType.STATICANNOUNCEMENTLIST).getProcessingTime();
}

@Managed
@Nested
public TimedStat getStaticAnnouncementDeleteProcessingTime()
{
return staticAnnouncementDeleteProcessingTime;
return eventTypeStatsEnumMap.get(DiscoveryEventType.STATICANNOUNCEMENTDELETE).getProcessingTime();
}

@Managed
@Nested
public TimedStat getDynamicAnnouncementProcessingTime()
{
return dynamicAnnouncementProcessingTime;
return eventTypeStatsEnumMap.get(DiscoveryEventType.DYNAMICANNOUNCEMENT).getProcessingTime();
}

@Managed
@Nested
public TimedStat getDynamicAnnouncementDeleteProcessingTime()
{
return dynamicAnnouncementDeleteProcessingTime;
return eventTypeStatsEnumMap.get(DiscoveryEventType.DYNAMICANNOUNCEMENTDELETE).getProcessingTime();
}

public void addStats(DiscoveryEventType type, boolean success, long startTime)
{
if (success) {
switch (type) {
case STATICANNOUNCEMENT:
staticAnnouncementSuccessCount.getAndIncrement();
staticAnnouncementProcessingTime.addValue(Duration.nanosSince(startTime));
break;
case STATICANNOUNCEMENTLIST:
staticAnnouncementListSuccessCount.getAndIncrement();
staticAnnouncementListProcessingTime.addValue(Duration.nanosSince(startTime));
break;
case STATICANNOUNCEMENTDELETE:
staticAnnouncementDeleteSuccessCount.getAndIncrement();
staticAnnouncementDeleteProcessingTime.addValue(Duration.nanosSince(startTime));
break;
case DYNAMICANNOUNCEMENT:
dynamicAnnouncementSuccessCount.getAndIncrement();
dynamicAnnouncementProcessingTime.addValue(Duration.nanosSince(startTime));
break;
case DYNAMICANNOUNCEMENTDELETE:
dynamicAnnouncementDeleteSuccessCount.getAndIncrement();
dynamicAnnouncementDeleteProcessingTime.addValue(Duration.nanosSince(startTime));
break;
case SERVICEQUERY:
serviceQuerySuccessCount.getAndIncrement();
serviceQueryProcessingTime.addValue(Duration.nanosSince(startTime));
break;
}
eventTypeStatsEnumMap.get(type).incrementCount(success);
eventTypeStatsEnumMap.get(type).addProcessingTime(startTime);
}

private static class Stats
{
private final AtomicLong successCount;
private final AtomicLong failureCount;
private final TimedStat processingTime;

public Stats()
{
this.successCount = new AtomicLong();
this.failureCount = new AtomicLong();
this.processingTime = new TimedStat();
}
else {
switch (type) {
case STATICANNOUNCEMENT:
staticAnnouncementFailureCount.getAndIncrement();
staticAnnouncementProcessingTime.addValue(Duration.nanosSince(startTime));
break;
case STATICANNOUNCEMENTLIST:
staticAnnouncementListFailureCount.getAndIncrement();
staticAnnouncementListProcessingTime.addValue(Duration.nanosSince(startTime));
break;
case STATICANNOUNCEMENTDELETE:
staticAnnouncementDeleteFailureCount.getAndIncrement();
staticAnnouncementDeleteProcessingTime.addValue(Duration.nanosSince(startTime));
break;
case DYNAMICANNOUNCEMENT:
dynamicAnnouncementFailureCount.getAndIncrement();
dynamicAnnouncementProcessingTime.addValue(Duration.nanosSince(startTime));
break;
case DYNAMICANNOUNCEMENTDELETE:
dynamicAnnouncementDeleteFailureCount.getAndIncrement();
dynamicAnnouncementDeleteProcessingTime.addValue(Duration.nanosSince(startTime));
break;
case SERVICEQUERY:
serviceQueryFailureCount.getAndIncrement();
serviceQueryProcessingTime.addValue(Duration.nanosSince(startTime));
break;

public long getSuccessCount()
{
return successCount.get();
}

public long getFailureCount()
{
return failureCount.get();
}

public TimedStat getProcessingTime()
{
return processingTime;
}

public void incrementCount(boolean success)
{
if (success) {
successCount.getAndIncrement();
}
else {
failureCount.getAndIncrement();
}
}

public void addProcessingTime(long startTime)
{
processingTime.addValue(Duration.nanosSince(startTime));
}
}
}
Expand Up @@ -93,7 +93,6 @@ public void testToString()
new DynamicServiceAnnouncement(Id.<Service>valueOf("50ad8530-2df6-4ff0-baa8-7a9c8d0abf51"), "type", Collections.<String, String>emptyMap()),
new DynamicServiceAnnouncement(Id.<Service>valueOf("5d528538-f907-4109-bbbd-8d609ab43225"), "type", Collections.<String, String>emptyMap()))
);

assertEquals(announcement.toString(), "DynamicAnnouncement{environment:testing,location:/location,pool:pool,services:[ServiceAnnouncement{id=50ad8530-2df6-4ff0-baa8-7a9c8d0abf51, type='type', properties={}}, ServiceAnnouncement{id=5d528538-f907-4109-bbbd-8d609ab43225, type='type', properties={}}]}");
assertEquals(announcement.toString(), "DynamicAnnouncement{environment=testing, pool=pool, location=/location, services=[ServiceAnnouncement{id=50ad8530-2df6-4ff0-baa8-7a9c8d0abf51, type='type', properties={}}, ServiceAnnouncement{id=5d528538-f907-4109-bbbd-8d609ab43225, type='type', properties={}}]}");
}
}

1 comment on commit 5577c22

@wyan0220
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi David, thanks for your comments. I've made the changes for most of them. Regarding dynamic proxy, i was told we should stay away from it if possible. For me I feel like it's a little overkill in this case...

Please sign in to comment.