Skip to content
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

Water Supply Accounting DTO #800

Open
wants to merge 40 commits into
base: develop
Choose a base branch
from

Conversation

zack-rma
Copy link
Collaborator

Implements Water Supply Accounting DTO with tests

@zack-rma zack-rma force-pushed the feature/water_supply_accounting_dto branch 4 times, most recently from 703dc80 to e9f2cf9 Compare July 24, 2024 21:30
@zack-rma zack-rma force-pushed the feature/water_supply_accounting_dto branch 2 times, most recently from 13a2e8a to 5ac239a Compare July 29, 2024 20:32
@zack-rma zack-rma force-pushed the feature/water_supply_accounting_dto branch 2 times, most recently from bc4642b to e8e85ba Compare August 14, 2024 15:51
@zack-rma zack-rma marked this pull request as ready for review August 14, 2024 15:59
@zack-rma
Copy link
Collaborator Author

@RyanM-RMA RyanM-RMA self-requested a review August 20, 2024 21:27
@zack-rma zack-rma force-pushed the feature/water_supply_accounting_dto branch from e7f8914 to 950e791 Compare August 23, 2024 17:41
Copy link
Collaborator

@rma-psmorris rma-psmorris left a comment

Choose a reason for hiding this comment

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

Feedback on code, mainly about how accounting needs to scale and is a time series.

private final String contractName;
@JsonProperty(required = true)
private final WaterUser waterUser;
private final List<PumpAccounting> pumpAccounting;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This data structure should be better defined. It should account for the different pumps and then the accounting times for a given pump. It should be able to easily answer without doing a full list scan whether a given pump has accounting, and what the given pump's start and end times are for the accounting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Restructured for less repeated data and better scaling

},
"water-right": "Test Water Right"
},
"pump-accounting": [
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have a lot of repeated data here w/in the pump accounting JSON. Accounting is a time series, and this portion of the data structure will need to scale.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rewrote for better scaling

"transfer-date": 10000012648000,
"comment": "Test Comment"
},
"2286-11-20T21:17:29Z": {
Copy link
Collaborator

@rma-psmorris rma-psmorris Sep 25, 2024

Choose a reason for hiding this comment

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

What is the difference between this date, "2286-11-20T21:17:29Z": { and the value in "transfer-date". Also, I'm not sure if these dates resolve down to the second w/in the database. You should check that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The dates are the same - it's duplicate data. It should be removed in the next iteration of the DTO based on the timeseries-style JSON format currently being tested

@JsonProperty(required = true)
private final CwmsId pumpLocation;
@JsonProperty(required = true)
private final Map<Instant, PumpTransfer> pumpTransfers;
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid repeated dates, you could drop this date (stop using a Map) and use a navigable set with a comparator on the pump transfer date.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll consider doing that with the new DTO structure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants