Added data operation failure table and a partially succesful status X… #388
Conversation
| @@ -234,6 +234,7 @@ private void Process(FileGroup fileGroup, Meter meter) | |||
| configurator(reader); | |||
|
|
|||
| MeterDataSet meterDataSet = reader.Parse(fileGroup); | |||
| FileGroupProcessingStatus processingStatus = FileGroupProcessingStatus.Failed; | |||
There was a problem hiding this comment.
Previously if (meterDataSet is null) it would got into FileGroupProcessingStatus.Processed state while now it will remain in Failed that seems to be an issue. Technically I am not sure who frequent that is since it should not reach the AnalysisNode in most cases but I think there is a potential for issues here.
| try | ||
| { | ||
| if (IsDisposed) |
There was a problem hiding this comment.
I am pretty sure this will generate multiple failures now. Previously IsDisposed caused it to exit the Process function completly. Now it will move on to the next dataOperationDefinition . Because IsDisposed is global for all dataOperationDefinigitons we end up with the same error message multiple times. at the very least we should prevent that
There was a problem hiding this comment.
It will still exit the function completely, but only after logging the exception in the data operation failure. When we catch the exception on line 319, there is a block that checks IsDisposed on line 334. If we are, we return out of the function, and never move onto another definition.
| if (index == 0) status = FileGroupProcessingStatus.Success; | ||
| else if (status == FileGroupProcessingStatus.Failed) status = FileGroupProcessingStatus.PartialSuccess; |
There was a problem hiding this comment.
This logic seems off.
If it has 3 dataOperationDefinitions (and all suceed) it will go:
from Failure to Success in index = 0
not change in index = 1 and index = 2
But if the last one fails it will go
from Failure to Success in index = 0
not change in index = 1 and
from Success to Failure in index = 2 which is not correct.
There was a problem hiding this comment.
With the current code, on failure it should either
Be in Failure or Partial Success, in either case things shouldn't change.
Be in Success and go to Partial Success, as is the case on line 323.
In the case you highlighted, the code I believe it is doing the following.
Go from Failure to Success in index = 0 (line 315)
Not change in index = 1 (line 315-316, neither condition should be met)
Go from Success to Partial Success in index = 2 (line 323)
| StackTrace = ex.StackTrace, | ||
| TimeOfFailure = DateTime.UtcNow | ||
| }); | ||
| if (IsDisposed) |
There was a problem hiding this comment.
If we dispose we need to consider the entire Processing Failed.
PartialSuccess should only apply to failures inside
meterDataSet.Configure(dataOperation);
dataOperation.Execute(meterDataSet);
If the underlying infrastiurcture AnalysisNode fails that is considered a complete failure since It will not even attepmt to run the rest of the analytics
There was a problem hiding this comment.
This makes sense, updated.
| } | ||
|
|
||
| FileGroup fileGroup = meterDataSet.FileGroup; |
There was a problem hiding this comment.
removing that seems problematic - though it could be a result of Github not aligning changes well. Is that still in there somewhere
There was a problem hiding this comment.
It got moved up into line 275
| private void LogDataOperationFailures(IEnumerable<DataOperationFailure> failures) | ||
| { | ||
| using (AdoDataConnection connection = CreateDbConnection()) | ||
| { | ||
| TableOperations<DataOperationFailure> failureTable = new TableOperations<DataOperationFailure>(connection); | ||
| foreach (DataOperationFailure failure in failures) | ||
| failureTable.AddNewRecord(failure); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
We need to make sure if that table is missing it does not take everything down with it.
Probably should tunr this into TryLogDataOperationFailures and wrap everything in a try/catch with a standard log if it fails.
There was a problem hiding this comment.
This is a good suggestion, thank you.
XDA-49