-
Notifications
You must be signed in to change notification settings - Fork 12
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
Решение проблемы с батч-запросом #294
Conversation
NewPlatform.Flexberry.ORM.ODataService/Extensions/DataServiceExtensions.cs
Outdated
Show resolved
Hide resolved
Tests/NewPlatform.Flexberry.ORM.ODataService.Tests/CRUD/BatchTest.cs
Outdated
Show resolved
Hide resolved
/// <param name="dataObjectCacheActual">Текущий основной кэш объектов.</param> | ||
/// <param name="dataObjectCacheWithMasters">Вспомогательный кэш, куда загружался объект.</param> | ||
/// <param name="loadedDataObject">Свежезагруженный объект, по которому обновляется основной кэш.</param> | ||
/// <param name="loadedObjectsAdded">Флаг, определяющий, что в кэш уже добавлен свежезагруженный объект.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вроде бы нет ситуации, когда это не так. Предлагаю убрать лишний параметр
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Этот параметр акцентирует на этом внимание.
Ещё несколько мест сомнительных есть, там этот флаг понадобится, поэтому пока оставила.
return; | ||
} | ||
|
||
if (!loadedObjectsAdded) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А нельзя вместо проверки флага проверить есть ли объект в кеше и добавить если нет?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это мини-оптимизация. До вызова этого метода уже проверки проходили. Чтобы не лезть повторно в кэш попусту, сделано так.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Проверяли что она действительно дает значимый прирост скорости? Просто наличие подобного параметра, который к тому же можно некорректно передать (то бишь передать true, а в кеше объекта не будет), мне кажется должно ощутимую выгоду давать
|
||
Порода poroda1 = args.DataService.Query<Порода>(porodaDynamicView).FirstOrDefault(x => x.__PrimaryKey == poroda.__PrimaryKey); | ||
Кошка koshka1 = args.DataService.Query<Кошка>(koshkaDynamicView).FirstOrDefault(x => x.__PrimaryKey == koshka.__PrimaryKey); | ||
Assert.NotNull(poroda1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А почему Assert-ы внутри секции Arrange?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Мне показалось, что это наиболее разумный способ проверить, что тестовая среда создалась корректно. Делать такие проверки после Act нелогично.
Как в таком случае разумно поступать?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А зачем это проверять? Если создалась некорректно, тест упадет наверное и так, на других проверках
@@ -731,7 +737,7 @@ private DataObject ReturnDataObject(Type objType, object keyValue) | |||
View miniView = view.Clone(); | |||
DetailInView[] miniViewDetails = miniView.Details; | |||
miniView.Details = new DetailInView[0]; | |||
_dataService.LoadObject(miniView, dataObjectFromCache, false, true, DataObjectCache); | |||
_dataService.SafeLoadWithMasters(miniView, dataObjectFromCache, DataObjectCache); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А это не должно быть дефолтным поведением LoadObject-а? Если я в прикладном коде захочу подобный граф объектов загрузить я с такой же проблемой перетирания измененных полей объектов в кеше столкнусь?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут вопрос в том, как ты и куда будешь грузить. Обычно LoadObject на прикладном уровне просто берёт объекты из БД. А при догрузке объекта уже прикладной разработчик с учётом вьюшки колдует, чтобы всё правильно было.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Обычно да, но на прикладном уровне и кеш обычно не передают. А вот если передают, то наверное ожидается что объекты в кеше не попортятся
На рассмотрении был батч-запрос, где приходила сложная иерархическая структура с наследованиями, детейлами и мастеровыми связями между детейлами. В результате выполнения батч-запроса некорректно перетирались кэши, от чего происходил сбой выполнения.
Исправление более аккуратно производит догрузку мастеров (ранее они могли быть перетёрты), а также при догрузке детейлов совсем не смешивает кэши (что приводило к очень отдалённым последствиям).