5
\$\begingroup\$

We have a console application which runs to perform an incremental load of our data warehouse staging area, with data from a number of production databases. The source database don't necessarily have a rowversion/timestamp/etc. column so we are currently using change data tracking in SQL to load changes to the tables. This all works fine and I think most of the code is okay, however, I don't like the way the 'main' pipeline works and was wondering if anyone had any better suggestions.

At the moment I have each 'step' as an instance of a class like this. This is so I can show a status of each step and if errors happen we have a descriptive name of the action it was performing. I was also thinking of adding pre and post conditions to each step so we can more easily tell the reason it fails.

public abstract class PipeLineStep
{
    public abstract String StepName { get; }
    public bool WasSuccessful { get; set; }
    internal abstract void Run();
}

These steps are run by the following method to enable a common type of logging. If an error is detected in the run, and we are not performing a full load, then the process will run again but perform a full load.

private bool RunStep(PipeLineStep step, TableContext context, Boolean isFullLoad, Boolean errorRerun)
{
    try
    {
        step.Run();
        return true;
    }
    catch (Exception ex)
    {
        Logging.Log(false, String.Format("Error during Pipeline Step [{0}]. {1}", step.StepName, ex.Message), context.TaskId, context.TableId, step.StepName, true, CurrentRunNumber);
        Logging.Log(false, String.Format("{0}", !isFullLoad && !errorRerun ? "Attempting Full Load of Table." : "Aborting Table Load."), context.TaskId, context.TableId, step.StepName, true, CurrentRunNumber);

        // If not currently doing a full load and we have an error - trying doing full load to refresh schema
        if (!isFullLoad && !errorRerun)
        {
            Run(context, CurrentRunNumber, isFullLoad, true);
            return false;
        }

        UpdateProgress(context.TableId, "Finished with Error", true, true);
        return false;
    }
}

In the main loop this method is called for each step, with control flow logic:

var step1 = new CheckChangeTrackingEnabledOnDatabaseStep(...);
if (!RunStep(step1, context, isFullLoad, errorRerun)) return false;

I don't see the actual order or control flow changing too much because each step is fairly well defined and we would handle any additional processing after the data is loaded into the staging area. So this part does not need to be too flexible.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answers, I will look through them later today. If I modify the code should I edit this question or post a new one?? \$\endgroup\$ – Milney Jun 28 '16 at 12:01
  • \$\begingroup\$ Post a new one please see the help centre for why we prefer that :) \$\endgroup\$ – RobH Jun 28 '16 at 14:38
10
\$\begingroup\$

I'd need more context so please consider some suggestions as implicit questions.

In PipeLineStep class StepName property is a read-only property. If you just return a constant value you may want to make it a constructor parameter instead (where you may also include some sort of validation):

protected PipeLineStep(string name) {
    if (String.IsNullOrWhiteSpace(name))
        throw new ArgumentException("Step name must not be nor empty.", "name");

    StepName = name;
}

public string StepName {
    get;
    private set;
}

Property name itself is little bit redundant, you do not need to repeat Step* for a property of a class named *Step:

public string Name {
    get;
    private set;
}

In the same class WasSuccessful is a public read/write property. Does anyone (inside or outside its assembly) has right to both read and write this property? Assuming (because Run() is internal) that also WasSuccessful is set from within same assembly then I'd make its setter internal.

In the same class Run() method is internal. Because it's also abstract it seems that it's used only from the same assembly where class is declared and also that all derived classes are located in the same assembly. This arises a question: does it make sense to have a public abstract class that can be used and extended only from the same assembly? It may be - if other assemblies will access Name ans WasSuccessful properties - but if it's not then don't make this class public.

In general visibility issues won't make your code broken but they will help to make it safer (from usage point of view) and they communicate intent.


In RunStep method you're catching a generic Exception. You should formalize which exceptions each step will throw to possibly narrow down to a smaller set of exceptions, catching Exception you get everything, from OutOfMemoryException to ArgumentException and it's bad because they're exceptions that need a special handling. Also, knowing what you're catching will help you to determine corner cases and error scenarios: a very good exercise for you and for your tests.

You're mixing bool and Boolean (for example), they're equivalent then I'd suggest to use the style you prefer and be consistent, reader won't repeatedly ask himself why (variations are eye-catching, they must be justified).

Parameter isFullLoad is not meaningful without more context, I'd rename it to something else but I actually don't know what it means (and your future-you won't know either without re-reading surrounding code...)

You're using a parameter errorRerun to know if you're retrying because of a previous error. While it's not an issue in such simple scenario it may be confusing (see your handling logic) and it adds more parameters to a function that should be as simple (you already have four parameters, two/three more than what's good). I can't suggest anything without much more context and knowledge but it's a point I'd investigate further.

Expression is !isFullLoad && !errorRerun is used twice, it's a good chance to extract a local variable with a meaningful name (again, rename it according to its exact meaning):

bool canRunFullPipelineAgain = !fullLoad && !errorRerun;

In general you're using too many function arguments and too many of them are bool. Pick any line where you use them and you can't understand their meaning without jumping back and forward to called function prototype (see RunStep(), Run(), UpdateProgress() and Logging.Log().)

To reduce number of arguments you have various techniques for example (again it depends on surrounding code) errorRerun may be a class member property as context and isFullLoaded (you have classes, containers of methods and state, you don't need to pass everything as parameters). Your logging function is a good example of this: its prototype is waaaaay too complex and convoluted. If you really can't change it - at least - use named parameters.

Boolean parameters are easier to remove, you may use enum instead of them (eventually - when it makes sense - combined as Flags). For example:

var options = RunStepOptions.TableIsFullyLoaded | RunStepOptions.RetryOnError);
if (!RunStep(step1, context, options))
    return false;

Minor question: shouldn't PipeLine be Pipeline?


Free thinking: if Run() executes all steps in pipeline then why you're putting error handling in RunStep()? Isn't easier to write a method Run() without error handling and an outer method RunWithErrorHandling() which takes care of this? You may also introduce a StepExecutionException, almost pseudo-code:

private void RunWithErrorHandling() {
    try {
       Run();
    }
    catch (StepExecutionException e) {
        // Try again? I don't know your full logic here
        Run();
    }
}

private void Run() {
    foreach (var step in Steps)
        RunStep(step);
}

private void RunStep(PipelineStep step) {
    try {
    }
    catch (Exception e) { // !!!
        // Logging?
        throw new StepExecutionException("...", e);
    }
}

Without surrounding code and better understanding of its context I can't suggest another design, just take this as something to think about.

\$\endgroup\$
7
\$\begingroup\$

I'll only focus on a single point:

Why is the return type of Run void?

Consider what the rest of your API tells me: WasSuccessful - interesting... Is that defined before a step is run? I doubt it. You're also relying on exceptions for control flow:

try
{
    DoSomething();
    return true;
}
catch 
{
    // Do some stuff.
    return false;
}  

Is a massive code smell for me. Why not wrap it all up in a return type for run?

public class PipelineStepResult
{
    public bool WasSuccessful { get; set; }

    public IEnumerable<string> Messages { get; set; }
}

Or whatever you need. Then you simply change Run to:

internal abstract PipelineStepResult Run();

That in turn lets you catch exceptions for something really exceptional but use the result for control flow.


Aside: "pipeline" is one word so you should be using PipelineStep.

\$\endgroup\$
2
\$\begingroup\$

I'll add one comment since I didn't see the other reviews cover it. One of the method signatures is:

private bool RunStep(PipeLineStep step, TableContext context, Boolean isFullLoad, Boolean errorRerun)

And internally, if an exception is caught, you end up checking the value of isFullLoad and potentially calling:

Run(context, CurrentRunNumber, isFullLoad, true);

This is a type of control coupling that makes me uncomfortable when I see my peers do it.

A pattern I would suggest instead is one where you deliberately execute a "full load" vs. a non full load.

E.g.

private bool RunStep(PipeLineStep step, TableContext context, Boolean errorRerun)
private bool RunStepAsFullLoad(PipeLineStep step, TableContext context, Boolean errorRerun)

Perhaps more importantly, that a call to Run() is INSIDE the exception handler also concerns me. As an example; What if that internal call to Run throws an exception? And if your answer is "I'll wrap that up in exception handling too" then I would be displeased, and suggest you consider the first suggestion in my first critique and allow the caller to handle this logic (which could mean another class I realize, but it makes more sense to me).

\$\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.