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.

Sign up for GitHub

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

[MB-6628] Use EagerPreload to improve performance in some key spots #5880

Merged
merged 6 commits into from
Feb 11, 2021
Merged

[MB-6628] Use EagerPreload to improve performance in some key spots #5880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aa3f68f
Using EagerPreload to optimize fetch-mto-updates
reggieriser Feb 9, 2021
6e2df41
Using EagerPreload to optimize TOO queue
reggieriser Feb 9, 2021
c1bd499
Doing separate Pop Load call to workaround EagerPreload bug
reggieriser Feb 10, 2021
b6c4cc4
Added error check; removed duplicate error processing
reggieriser Feb 10, 2021
ace8ff6
Moved payment request queue to EagerPreload; still working around Pop…
reggieriser Feb 10, 2021
b02a01c
Merge branch 'master' into rr-MB-6628-eagerpreload
reggieriser Feb 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/models/order.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ type Order struct {
HasDependents bool `json:"has_dependents" db:"has_dependents"`
SpouseHasProGear bool `json:"spouse_has_pro_gear" db:"spouse_has_pro_gear"`
NewDutyStationID uuid.UUID `json:"new_duty_station_id" db:"new_duty_station_id"`
NewDutyStation DutyStation `belongs_to:"duty_stations"`
NewDutyStation DutyStation `belongs_to:"duty_stations" fk_id:"new_duty_station_id"`
UploadedOrders Document `belongs_to:"documents"`
UploadedOrdersID uuid.UUID `json:"uploaded_orders_id" db:"uploaded_orders_id"`
OrdersNumber *string `json:"orders_number" db:"orders_number"`
Expand All @@ -56,7 +56,7 @@ type Order struct {
Grade *string `json:"grade" db:"grade"`
Entitlement *Entitlement `belongs_to:"entitlements"`
EntitlementID *uuid.UUID `json:"entitlement_id" db:"entitlement_id"`
OriginDutyStation *DutyStation `belongs_to:"duty_stations"`
OriginDutyStation *DutyStation `belongs_to:"duty_stations" fk_id:"origin_duty_station_id"`
Copy link
Contributor Author

@reggieriser reggieriser Feb 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I added the fk_id struct tags here to workaround a Pop issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for documenting it! 🥳

OriginDutyStationID *uuid.UUID `json:"origin_duty_station_id" db:"origin_duty_station_id"`
}

Expand Down
32 changes: 18 additions & 14 deletions pkg/services/move_order/move_order_fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,12 @@ func (f moveOrderFetcher) ListMoveOrders(officeUserID uuid.UUID, params *service
// Adding to an array so we can iterate over them and apply the filters after the query structure is set below
options := []QueryOption{branchQuery, locatorQuery, dodIDQuery, lastNameQuery, dutyStationQuery, moveStatusQuery, gblocQuery, sortOrderQuery}

query := f.db.Q().Eager(
query := f.db.Q().EagerPreload(
"Orders.ServiceMember",
"Orders.NewDutyStation.Address",
"Orders.OriginDutyStation",
"Orders.OriginDutyStation.Address",
// See note further below about having to do this in a separate Load call due to a Pop issue.
// "Orders.OriginDutyStation.TransportationOffice",
"Orders.Entitlement",
"MTOShipments",
"MTOServiceItems",
Expand All @@ -79,14 +81,6 @@ func (f moveOrderFetcher) ListMoveOrders(officeUserID uuid.UUID, params *service
}
}

if err != nil {
switch err {
case sql.ErrNoRows:
return []models.Move{}, 0, services.NotFoundError{}
default:
return []models.Move{}, 0, err
}
}
Comment on lines -82 to -89
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed this appears to be a leftover. The err is checked before this, and checked again after the next Pop call below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed the if statement check on line 9 doesn't look for ErrNoRows to return a NotFoundError and was wondering if it should.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, which if statement are you looking at? Line 9 is in the import section, so I'm guessing that's just a typo.

Copy link
Contributor

Choose a reason for hiding this comment

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

I must have miss read the line numbers. I believe I meant the one on line 32 as it doesn't return the NotFoundError like the one you deleted.

	if err != nil {
		return []models.Move{}, 0, err
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. That query is just getting the office user's associated transportation office's GBLOC to use as a filter in the next query for the TOO queue data. It should always return a record unless something really funky is going on because all the parts of that association are required in the DB, so not specifically looking for ErrNoRows is probably OK. That seems different from the big TOO queue query which might return no rows as part of normal operation because there doesn't happen to be anything available in that GBLOC's TOO queue.

// Pass zeros into paginate in this case. Which will give us 1 page and 20 per page respectively
if params.Page == nil {
params.Page = swag.Int64(0)
Expand Down Expand Up @@ -114,11 +108,21 @@ func (f moveOrderFetcher) ListMoveOrders(officeUserID uuid.UUID, params *service
count := query.Paginator.TotalEntriesSize

for i := range moves {
// Due to a bug in pop (https://github.com/gobuffalo/pop/issues/578), we
// cannot eager load the address as "OriginDutyStation.Address" because
// OriginDutyStation is a pointer.
// There appears to be a bug in Pop for EagerPreload when you have two or more eager paths with 3+ levels
// where the first 2 levels match. For example:
// "Orders.OriginDutyStation.Address" and "Orders.OriginDutyStation.TransportationOffice"
Copy link
Contributor

@shimonacarvalho shimonacarvalho Feb 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if we could provide an endpoint to GetDutyStation and avoid the 3+ level nesting in general. DutyStation addresses don't change too much so it's not a huge deal not to have all the data in the fetchMTOUpdate. We could just return name and id. Not necessary change for this pr but wondered what you thought.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting bug, I think we have a few other places that such things happen so it's good to have documented. That said having a get duty station end point might make sense given they won't change much and it seems likely the prime will have a list of them as well. Totally agree that this isn't something we need for this PR but worth considering.

@shimonacarvalho How much do we expect the prime to call fetch-mto-updates given they will be sent the information via the webhook notification. Or will they need to call it after receiving the notification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we could provide an endpoint to GetDutyStation and avoid the 3+ level nesting in general. DutyStation addresses don't change too much so it's not a huge deal not to have all the data in the fetchMTOUpdate. We could just return name and id. Not necessary change for this pr but wondered what you thought.

So this particular query is fetching the data used in the TOO queue, not fetch-mto-updates. I actually didn't have to do any Load workarounds in fetch-mto-updates, but I do have a couple of thoughts:

  • I do think it's reasonable to re-evaluate the data returned by fetch-mto-updates and perhaps prune it some. Another endpoint (I think there's a support one already, but we'd need something the prime could access) could get the full tree of data for a specific MTO, but fetch-mto-updates would just return a higher-level overview. But it depends how we expect the prime to use it.
  • I haven't done the forensics on this particular fetcher, but I think we sometimes create query functions/services that over time have more eager loading added to them as other use cases come up. The problem is that not every use case needs all this data so some cycles get wasted fetching data that's never used in some cases.

For this PR, I mainly focused on optimizing what was already there and intentionally not changing the data returned to avoid accidentally breaking something else in the process.

Copy link
Contributor

@shimonacarvalho shimonacarvalho Feb 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

So this particular query is fetching the data used in the TOO queue, not fetch-mto-updates

Oh my bad.

could get the full tree of data for a specific MTO, but fetch-mto-updates would just return a higher-level overview.

Yes, I just fixed up getMTO and it's really good for returning everything in a single mto. Easy to move from support --> prime.

I think the challenge with fetchMTOUpdates is that its real value to the overall API is returning mtos where ANY data on the mto has changed since a certain date. However that is not working right now, it doesn't "deep-check" nested updatedAts. So it really needs a full rethink/rewrite which we haven't been able to get prioritization on.

@shimonacarvalho How much do we expect the prime to call fetch-mto-updates given they will be sent the information via the webhook notification. Or will they need to call it after receiving the notification?

@gidjin I just don't know honestly, since we have no Prime to ask. But i believe we need to make it work properly, with the since feature. Because even if push notifications works well and they get all the data they need normally, at some point when push fails (a cert expires or whatever) they will have to resync using fetch.

// In those cases, only the last relationship is loaded in the results. So, we can only do one of the paths
// in the EagerPreload above and request the second one explicitly with a separate Load call.
//
// Note that we also had a problem before with Eager as well. Here's what we found with it:
// Due to a bug in pop (https://github.com/gobuffalo/pop/issues/578), we
// cannot eager load the address as "OriginDutyStation.Address" because
// OriginDutyStation is a pointer.
if moves[i].Orders.OriginDutyStation != nil {
f.db.Load(moves[i].Orders.OriginDutyStation, "Address", "TransportationOffice")
loadErr := f.db.Load(moves[i].Orders.OriginDutyStation, "TransportationOffice")
if loadErr != nil {
return []models.Move{}, 0, err
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/services/move_task_order/move_task_order_fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func (f moveTaskOrderFetcher) ListMoveTaskOrders(moveOrderID uuid.UUID, searchPa
func (f moveTaskOrderFetcher) ListAllMoveTaskOrders(searchParams *services.ListMoveTaskOrderParams) (models.Moves, error) {
var moveTaskOrders models.Moves
var err error
query := f.db.Q().Eager(
query := f.db.Q().EagerPreload(
"PaymentRequests.PaymentServiceItems.PaymentServiceItemParams.ServiceItemParamKey",
"MTOServiceItems.ReService",
"MTOServiceItems.Dimensions",
Expand Down
25 changes: 17 additions & 8 deletions pkg/services/payment_request/payment_request_list_fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,10 @@ func (f *paymentRequestListFetcher) FetchPaymentRequestList(officeUserID uuid.UU
}

paymentRequests := models.PaymentRequests{}
query := f.db.Q().Eager(
"MoveTaskOrder.Orders.OriginDutyStation",
"MoveTaskOrder.Orders.ServiceMember",
query := f.db.Q().EagerPreload(
"MoveTaskOrder.Orders.OriginDutyStation.TransportationOffice",
// See note further below about having to do this in a separate Load call due to a Pop issue.
// "MoveTaskOrder.Orders.ServiceMember",
).
InnerJoin("moves", "payment_requests.move_id = moves.id").
InnerJoin("orders", "orders.id = moves.orders_id").
Expand Down Expand Up @@ -103,11 +104,19 @@ func (f *paymentRequestListFetcher) FetchPaymentRequestList(officeUserID uuid.UU
}

for i := range paymentRequests {
// Due to a bug in pop (https://github.com/gobuffalo/pop/issues/578), we
// cannot eager load the address as "OriginDutyStation.Address" because
// OriginDutyStation is a pointer.
if originDutyStation := paymentRequests[i].MoveTaskOrder.Orders.OriginDutyStation; originDutyStation != nil {
f.db.Load(originDutyStation, "TransportationOffice")
// There appears to be a bug in Pop for EagerPreload when you have two or more eager paths with 3+ levels
// where the first 2 levels match. For example:
// "MoveTaskOrder.Orders.OriginDutyStation.TransportationOffice" and "MoveTaskOrder.Orders.ServiceMember"
// In those cases, only the last relationship is loaded in the results. So, we can only do one of the paths
// in the EagerPreload above and request the second one explicitly with a separate Load call.
//
// Note that we also had a problem before with Eager as well. Here's what we found with it:
// Due to a bug in pop (https://github.com/gobuffalo/pop/issues/578), we
// cannot eager load the address as "OriginDutyStation.Address" because
// OriginDutyStation is a pointer.
loadErr := f.db.Load(&paymentRequests[i].MoveTaskOrder.Orders, "ServiceMember")
if loadErr != nil {
return nil, 0, err
}
}

Expand Down