New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gather all endpoints and sort before logging them #1002

Merged
merged 1 commit into from Apr 21, 2015

Conversation

Projects
None yet
4 participants
@softarn
Contributor

softarn commented Apr 20, 2015

The list of endpoints logged when starting Dropwizard becomes hard to read when it grows larger. This will sort all endpoints before logging them.

rc.register(TestResource.class);
rc.register(ImplementingResource.class);

System.out.println(rc.getEndpointsInfo());

This comment has been minimized.

@klette

klette Apr 20, 2015

Contributor

Should probably be removed :)

This comment has been minimized.

@softarn

softarn Apr 20, 2015

Contributor

thanks, updated

@softarn softarn force-pushed the softarn:sorted-endpoints-logging branch 4 times, most recently from 5bb61da to 845b737 Apr 20, 2015

}
}

private static class EndpointComparator implements Comparator<EndpointLogLine>, Serializable {

This comment has been minimized.

@arteam

arteam Apr 20, 2015

Member

Could we just make EndpointLogLine to implement Comparable<EndpointLogLine>?
If it so, we won't need to specify a comparator explicitly.

This comment has been minimized.

@softarn

softarn Apr 20, 2015

Contributor

So much boiler code for equals, hashcode. That's why I chose the
comparator, want me to switch anyway?
On Apr 20, 2015 9:31 PM, "Artem Prigoda" notifications@github.com wrote:

In
dropwizard-jersey/src/main/java/io/dropwizard/jersey/DropwizardResourceConfig.java
#1002 (comment):

  •    public String getHttpMethod() {
    
  •        return httpMethod;
    
  •    }
    
  •    public String getBasePath() {
    
  •        return basePath;
    
  •    }
    
  •    @Override
    
  •    public String toString() {
    
  •        return String.format("    %-7s %s (%s)", httpMethod, basePath, klass.getCanonicalName());
    
  •    }
    
  • }
  • private static class EndpointComparator implements Comparator, Serializable {

Could we just make EndpointLogLine to implement
Comparable?
If it so, we won't need to specify a comparator explicitly.


Reply to this email directly or view it on GitHub
https://github.com/dropwizard/dropwizard/pull/1002/files#r28720803.

This comment has been minimized.

@arteam

arteam Apr 20, 2015

Member

That's a reasonable argument. Let's leave it then.


@Override
public int compare(EndpointLogLine endpointA, EndpointLogLine endpointB) {
int basePathComparison = endpointA.getBasePath().compareTo(endpointB.getBasePath());

This comment has been minimized.

@arteam

arteam Apr 20, 2015

Member

What do you think about using Guava's ComparisonChain? Something like this:

return ComparisonChain.start()
                    .compare(this.basePath, other.basePath)
                    .compare(this.httpMethod, other.httpMethod)
                    .result();

This comment has been minimized.

@softarn

softarn Apr 20, 2015

Contributor

Thanks for tip, didn't know about it I'll check it out!
On Apr 20, 2015 9:35 PM, "Artem Prigoda" notifications@github.com wrote:

In
dropwizard-jersey/src/main/java/io/dropwizard/jersey/DropwizardResourceConfig.java
#1002 (comment):

  •    public String getBasePath() {
    
  •        return basePath;
    
  •    }
    
  •    @Override
    
  •    public String toString() {
    
  •        return String.format("    %-7s %s (%s)", httpMethod, basePath, klass.getCanonicalName());
    
  •    }
    
  • }
  • private static class EndpointComparator implements Comparator, Serializable {
  •    private static final long serialVersionUID = 1L;
    
  •    @Override
    
  •    public int compare(EndpointLogLine endpointA, EndpointLogLine endpointB) {
    
  •        int basePathComparison = endpointA.getBasePath().compareTo(endpointB.getBasePath());
    

What do you think about using Guava's ComparisonChain
https://github.com/google/guava/blob/master/guava/src/com/google/common/collect/ComparisonChain.java
?
Something like this:

return ComparisonChain.start()
.compare(this.basePath, other.basePath)
.compare(this.httpMethod, other.httpMethod)
.result();


Reply to this email directly or view it on GitHub
https://github.com/dropwizard/dropwizard/pull/1002/files#r28721112.

@@ -110,6 +112,8 @@ public void setUrlPattern(String urlPattern) {

public String getEndpointsInfo() {
final StringBuilder msg = new StringBuilder(1024);
final Set<EndpointLogLine> endpointLogLines = Sets.newTreeSet(new EndpointComparator());

This comment has been minimized.

@arteam

arteam Apr 20, 2015

Member

Do we really need a TreeSet?
Basically we don't have duplicates and don't care about speed of lookup. The only thing that we need, it's a sorted collection. We could use an ordinary ArrayList with some expected size to avoid resizing and then sort it. The collection is relatively small, so sorting should be fast.

TreeSet has a very big memory footprint and it's not cache-friendly. I would not use it, if we have alternatives.

@@ -139,7 +146,7 @@ public String getEndpointsInfo() {
*/
private static class EndpointLogger {
private final String rootPath;
private final List<String> endpoints = Lists.newArrayList();
private final List<EndpointLogLine> endpointLogLines = Lists.newArrayList();

This comment has been minimized.

@arteam

arteam Apr 20, 2015

Member

Could we pass a link to the root collection, rather then creating a new collection for every resource?
Now it's created only to be captured.

this.httpMethod = httpMethod;
}

public String getHttpMethod() {

This comment has been minimized.

@arteam

arteam Apr 20, 2015

Member

We could access these fields directly from the comparator, so these getters are not needed.

rc.register(ImplementingResource.class);

assertThat(rc.getEndpointsInfo())
.contains(" GET / (io.dropwizard.jersey.dummy.DummyResource)\n"

This comment has been minimized.

@arteam

arteam Apr 20, 2015

Member

This test will fail on Windows, because it has different line breaks (\r\n rather than \n).
Could we wrap an expected string in String.format or use a regular expression?

@arteam

This comment has been minimized.

Member

arteam commented Apr 20, 2015

@softarn, thanks for the contribution!
I've added some notes about where the PR could be improved, but in overall it looks great.

@softarn softarn force-pushed the softarn:sorted-endpoints-logging branch from 845b737 to e547edb Apr 21, 2015

@softarn

This comment has been minimized.

Contributor

softarn commented Apr 21, 2015

@arteam I've updated the commit. Thank you for the comments!

arteam added a commit that referenced this pull request Apr 21, 2015

Merge pull request #1002 from softarn/sorted-endpoints-logging
Gather all endpoints and sort before logging them

@arteam arteam merged commit b073dab into dropwizard:master Apr 21, 2015

@arteam

This comment has been minimized.

Member

arteam commented Apr 21, 2015

Thanks, @softarn, for your efforts on that.

@jplock jplock added this to the 0.9.0 milestone Apr 27, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment