2
\$\begingroup\$

The inspiration for this question comes from a talk by IanCooper; in particular, this remark:

Never mock somebody else's interface... you want to mock your abstraction of how you call the third party

I'm implementing a monitor for our hadoop cluster; something that examines TaskCompletionEvents and exposes certain classes of errors that we've been failing to detect up to this point.

To do this, my code needs to invoke three different methods available in the Hadoop library:

  • JobClient.getAllJobs()
  • JobClient.getJob()
  • RunningJob.getTaskCompletionEvents()

package com.stackexchange.codereview.question.adapters.hadoop.jobtracker;

import ...

public interface JobTrackerClient {
    JobStatus[] getAllJobs() throws IOException;

    Job getJob(JobID jobId) throws IOException;

    interface Job {
        TaskCompletionEvent[] getTaskCompletionEvents(int nextEvent) throws IOException;
    }
}

There's not a lot of abstraction going on here - it turns out that getTaskCompletionEvents needs to be called many times per job, and I want unit tests to verify my handling of various corner cases.

  1. What do people think is the right pattern to use for the name of the package in which the adapter lives?

  2. What should be the name of the adapter that actually implements this interface? Note that the implementation is a really thin skin around JobClient and RunningJob. Does the implementation live in the same package as the interface?

  3. Should the abstraction insulate the consumer from the specific exceptions being thrown?

  4. Should the abstraction also insulate the caller from 3rd party data objects? (ex: JobID, JobStatus, TaskCompletionEvent)

Here's an implementation of the adapter itself

public class JobTracker implements JobTrackerClient {
    private final JobClient jobClient;

    public JobTracker(JobClient jobClient) {
        this.jobClient = jobClient;
    }

    @Override
    public Job getJob(JobID jobId) throws IOException {
        final RunningJob runningJob = jobClient.getJob(jobId);

        return new Job() {
            @Override
            public TaskCompletionEvent[] getTaskCompletionEvents(int nextEvent) throws IOException {
                return runningJob.getTaskCompletionEvents(nextEvent);
            }
        };
    }

    @Override
    public JobStatus[] getAllJobs() throws IOException {
        return jobClient.getAllJobs();
    }

    // Factory methods for creating configured instances of the client.
    public static JobTrackerClient create() throws IOException {
        return create(new Configuration());
    }

    public static JobTrackerClient create(Configuration conf) throws IOException {
        JobConf jobConf = new JobConf(conf);

        JobClient client = new JobClient(jobConf);

        return create(client);
    }

    public static JobTrackerClient create(JobClient client) {
        return new JobTracker(client);
    }
}

Are there any arguments to prefer the following implementation?

@Override
public Job getJob(final JobID jobId) throws IOException {
    return new Job() {
        final RunningJob runningJob = jobClient.getJob(jobId);

        @Override
        public TaskCompletionEvent[] getTaskCompletionEvents(int nextEvent) throws IOException {
            return runningJob.getTaskCompletionEvents(nextEvent);
        }
    };
}
\$\endgroup\$
  • \$\begingroup\$ Why are you implementing your mocks by hand? Why not use a library like mockito? \$\endgroup\$ – Emily L. Aug 22 '14 at 20:40
  • \$\begingroup\$ I'm not implementing mocks by hand. I'm experimenting with the guideline that I should only be mocking my own interfaces. \$\endgroup\$ – VoiceOfUnreason Aug 22 '14 at 20:56
  • \$\begingroup\$ Have your written any test cases yet? If so, an example test case would be very useful for the arguments to be more concrete. Question is about mocking, but code examples do not contain any mocking. \$\endgroup\$ – abuzittin gillifirca Aug 29 '14 at 8:51
2
\$\begingroup\$

The benefit of using an adapter in this context (though i'm not sure it's an adapter, it feels more like a facade, even if it just sits in front of 1 class) is that you can translate the objects of the external system to your internal model.

1: I would suggest the pattern you use is a facade. as for package, I would put it in it's own package named "XXX.job" or "XXX.task" depending on which feels to fit better.

2: A number of articles have spoken out against splitting interfaces and implementations in separate packages. So take this with a grain of salt, but personally I like creating the facade in package "XXX.task" and then creating a hadoop implementation in "XXX.task.hadoop". That way you can create local definitions in the hadoop package, while everything generic (and therefor anything used by the rest of your application) should live in "XXX.task"

3: Yes, this is very important in my opinion. The facade would abstract the Job/Task functionality or system. It should not be clear from the outside whether this is a homegrown implementation or an external product. I would define Exceptions in the generic package, and in the implementation, wrap the hadoop exception in your exception.

4: Yes, this is to my knowledge considered best practice. You don't want the details of the specific scheduling implementation chosen to bleed through to the rest of the application. Again it will allow you more easily to switch implementations and it will make you more resilient to change.

The one time I've counted my blessings having used the above to abstract an external system, a week before end of development the remote team split some of their objects in 2 and forced us to do multiple calls to get the same info. Since we had the facade in place, this meant we only changed the facade implementation and nothing else.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.