Добрый день. В общем тут такое дело, устраивался недавно в одну контору, где в почете многопоточность. Экспертом себя никогда не считал, но и не полный нуб в этой теме. Дали тестовое задание сделать, сделал - отписались, мол вы нам не подходите, потому что "There were some critical errors in the task which in real multithreading environment wouldn't work correctly". Естественно без деталей:) И вот чето меня так цепануло по проф пригодности что места не могу найти. Если есть возможность, сделайте ревью кода и пните меня в эти критикал эррорс, ну и вообще жесточайшая критика приветствуется:). Задание в принципе не сложное - тривиальный сервис по обработке входящих данных, накоплению, и возвращению результата(ТЗ прилагается - там всего 2 листика). Напишите мне на мыло в профиле если есть желание(тема может быть интересна многим) - вышлю зипку с проектом, ну так чисто мозг размять. Проект мавеновский, из зависимостей только guice, кода немного - старался сделать хорошим). Если вы в одном городе со мной - щедро угостил бы пивом, но сомневаюсь что вы оттуда:)
В общем если есть желание - you are welcome.

Думал на форум не влезет - ан нет) Спасибо что откликнулись.

А, да еще пару уточнений - собирайте под java 1.6(изза @Override над методами интерфейсов) и я когда посылал задание уточнил - что в компоненте Storage -сознательно упростил некоторые вещи, вроде проверок на нулл, так как считаю что он внутренний по отношению к системе, и считаю его доверенным.

Пересмотрел еще раз - нашел таки одну ошибку:( Но ведь в ответе было несколько:) В любом случае комменты приветствуются, пока не буду говорить что нашел, чтоб было интересней.

Да, вы правы, сказывается отсутствие опыта по работе с финансами, доменную модель сделал сразу, а потом благополучно забыл про таймстамп. Сконцентрировался на строчке в ТЗ - "Quotes are coming in a natural order, that is timestamp of a next quote is always bigger than timestamp of a previous one." И наивно полагал что локальное время сервера - это таймстамп квоты, лажанул короче. Ну вот походу и вторая ошибка, спасибо - хоть полезное почерпнул.

Blazkowicz Пока trendbar не закрыт (например поток, который его закрывает, решил немного поголодать) все квоты идут в старый trendbar.



schwaЗакрытие в одном потоке, а апдейт в другом.
TrendBar




Что будет в closeClose на момент закрытия неизвестно.

Да, вот это все по сути и есть та ошибка, которую я нашел. Сначала делал однопоточную версию, а потом решил добавить асинхронность. Суть идеи была такова - на каждый Symbol - завести по экзекьютору с пулом в один поток, и сабмитить CloseTask и HandleTask в этот самый эзекьютор - тогда бы они гарантированно выполнялись по очереди и я бы экономил на синхронизации, но closeTask забыл доработать, а учитывая косяк с таймстампом, указанный выше - да, соглашусь это действительно критикал эрроры и код не будет работать корректно. Ну хоть душа спокойна :).
Сказывается что у меня не было реального проекта с многопоточностью, точнее был но я там был один и варился в собственном соку, вот и хотел уйти в эту область чтобы прокачаться - ну что ж, пока не судьба.

BlazkowiczЗакрытие нет смысла делать скедулерами. Пришла - квота. Проверили последний TrendBar, попадает в него - ОК. Не попадает, наделали пустых TrendBar-ов чтобы закрыть предыдущий период, и потом сделали текущий в него записались и всё.
В этом случае будет только с history проблема. Новые трендбары не будут наполнятся, пока не придёт квота. Сдругой стороны а нафига нужны пустые трендбары в системе. 8)

Да! Я выбирал между этими двумя вариантами кстати, остановился на своем как более логичном чтоли - чтобы код был красивее, и не было проблемы с хистори. Даже хотел вопрос им написать - но не написал).

Всем спасибо) Получил ценный опыт, есть еще комментарии по улучшению, по качеству кода и тп? Ну если по многопоточности еще что-то найдете - тоже кул)

BlazkowiczНе совсем про многопоточность но ещё сильно напрягает обращение с датами в Period + PeriodType.
Period.<init> - new Date()
PeriodType.calculateEndPeriodDate() - Calendar.newInstance(), чтобы отсчитать от new Date(), который создали выше?

Да - согласен, неинуитивно? Или все-таки неправильно? Как бы вы поступили? Долго думал над этим моментом.


насчет авторИ вот это озадачило окнчательно.
new Date(currentCalendar.getTime().getTime())

Наследие того что в java дату - мьютабл, машинально написал похоже, хотя может и была причина - если честно не помню)

BlazkowiczВариант с паралельным CloseTask никак не может быть проще.
Как сейчас - нужно синхронизировать гонки между CloseTask и HandleTask.
Даже если отказаться от ScheduledExecutor-а и обрабатывать CloseTask в единственном потоке TaskBar, то всё равно не понятно как его синхронизировать с очередью HandleTask. Потому что CloseTask надо впихнуть в правильное место очереди.

В этом и была идея - сделать ссылку с мапой экзекьюторов доступной для всех через инжекшен( на каждый Symbol по экзекьютору) - и сабмитить оба типа тасков их именно в один и тот же экзекьютор(брать по ключу symbol), извиняюсь может путано пишу.

schwaЕще. Тоже в закрытии.



У нас минутный интервал работает закрывается вместе с часовым и теряем мы просто trendBar. Ибо храним их в не в потокобезопастном LinkedList.

Я сейчас подумаю над этим - с ходу не понял, можете на пальцах пока я подумаю?

Blazkowiczзабыл никВсем спасибо) Получил ценный опыт, есть еще комментарии по улучшению, по качеству кода и тп? Ну если по многопоточности еще что-то найдете - тоже кул)
Очень напрягает обилие chained вызовов. Они ухудшают читаемость. А в случае NPE делают невозможным анализ лога.
storage.get(trendBar.getSymbol()).get(trendBar.getPeriodType()).add(trendBar)
5 вызовов методов. Угадай кто был null.
2 get метода - если там вдруг одинаковая коллекция и в ней произошло исключение, тоже нельзя сказать кто же из двух это был.

Да, но я делал описку, что упростил проверки на нулл в storage, так как считаю что он не шарится между системами(вроде как доверенный код), и гарантировано правильно инициализирован, но вообще учту, спасибо.

Blazkowiczзабыл никДа - согласен, неинуитивно? Или все-таки неправильно?

Оба.
неинуитивно (хотя может я уже под вечер не соображаю) - что делает calculateEndPeriodDate() я могу понять только из имени. Что делает код, сходу не понятно. Надо разбираться.
возможно неправильно - end date вычисляется от текущего момента, а не от start date. Почему так? Какая в этом особая задумка?


Да именно в этом и задумка - ведь нам нужен интервал от текущего времени ровно до следующей минуты\секунды чтобы зашедулить клоузтаск. А считать от startDate - неправильно, потому что при инициализации создается trendBar у которого startDate может быть, допустим, в середине минуты. Ну неинтуитивно -да..

Посмотри реализацию Calendar.getTime().
Спасибо!

Blazkowiczзабыл никДа, но я делал описку, что упростил проверки на нулл в storage, так как считаю что он не шарится между системами(вроде как доверенный код), и гарантировано правильно инициализирован, но вообще учту, спасибо.
А null может и не снаружи попасть. Сам где-нибудь образуется из-за баги с многопоточностью. :)

Так там все по идее иммьютабл и наполняется после инициализации - наверное не очевидно просто...

Blazkowiczзабыл никВ этом и была идея - сделать ссылку с мапой экзекьюторов доступной для всех через инжекшен( на каждый Symbol по экзекьютору) - и сабмитить оба типа тасков их именно в один и тот же экзекьютор(брать по ключу symbol), извиняюсь может путано пишу.
Да, я понял идею. Просто когда у тебя в ScheduledThreadPoolExecutor нарисовалась очередь из квот, как ты воткнешь CloseTask в нужную позицию?

Да, теперь когда вы указали на косяк с таймстампами в квоте - становится очевидно что этот вариант неправильный, а так по идее это разруливалось через натурал ордеринг) Типа если клоуз таск стартовал - то квот с таймстампами < now нет.. Ну в общем тут комплекс.

schwaschwaпропущено...

Ложная тревога.


Пропустил trendBar.getPeriodType(). Все будет нормально.
Но проблемы с видимостью содержимого List<TrendBar>, когда делаем запрос getTrendBars, все равно будут.
Тк там простой LinkedList.
Да и ConcurrentModificationException там можно будет словить.

Пожалуй что да, вы бы использовали CopyOnWriteArrayList? Но я еще об этом подумаю, ибо я помню что думал об этом, и пришел к выводу что все будет ок, задание делал давно - надо вспоминать, хотя вот сейчас посмотрел - вроде вы правы, если тут косяк - то вот его то я точно должен был не допускать, даже странно...
Спасибо!

BlazkowiczgetStartDate().equals(from) - правда? 1ms реально на что-то влияет?
Double price; - хорошо что сумму считать не попросили.
Так потому и дабл, раз не попросили - так бы был децимал:)

BlazkowiczgetStartDate().equals(from) - правда? 1ms реально на что-то влияет?

Ну вот тут хз - разве не влияет?

забыл никschwaпропущено...

Да и ConcurrentModificationException там можно будет словить.

Пожалуй что да, вы бы использовали CopyOnWriteArrayList? Но я еще об этом подумаю, ибо я помню что думал об этом, и пришел к выводу что все будет ок, задание делал давно - надо вспоминать, хотя вот сейчас посмотрел - вроде вы правы, если тут косяк - то вот его то я точно должен был не допускать, даже странно...
Спасибо!

Все же вы правы:)

В общем получается что ошибка с таймстампом была самой эпичной - не будь ее, сделал бы без шедулера, как Blazkowicz предложил, ну и LinkedList - но я думаю будь это единственной ошибкой - закрыли бы глаза. Ну и часть кода неинтуитивна - вот и найди тут баланс между простотой и очевидностью.

Blazkowiczзабыл ник,

getStartDate().equals(from) || getStartDate().after(from) == !getStartDate().before(from)

Да, так лучше) Хотя, возможно, для кого-то менее интуитивно, опять же к вопросу о балансе)

Blazkowiczзабыл никВ общем получается что ошибка с таймстампом была самой эпичной - не будь ее, сделал бы без шедулера, как Blazkowicz предложил, ну и LinkedList - но я думаю будь это единственной ошибкой - закрыли бы глаза. Ну и часть кода неинтуитивна - вот и найди тут баланс между простотой и очевидностью.
Я что-то не догнал про LinkedList. Поясните, плз.

ЛинкедЛист - непотокобезопасная коллекция, а там идет ремув, апдейт из разных потоков - могут быть проблемы с visibility, на x86 - врядли, но теоретически неправильно. Ну и при переборе через итератор - может возникнуть исключение ConcurrentModifying.. в отличие от CopyOnWrit'a

Blazkowiczзабыл никДа именно в этом и задумка - ведь нам нужен интервал от текущего времени ровно до следующей минуты\секунды чтобы зашедулить клоузтаск. А считать от startDate - неправильно, потому что при инициализации создается trendBar у которого startDate может быть, допустим, в середине минуты. Ну неинтуитивно -да..

Я всё равно не понял задумки.
Есть startDate и endDate. Оба вычисляются в зависимости от текущего времени. Т.е. длина не фиксирована и может быть любой?

startDate - когда трендбар реально начал мониториться( при начальной инициализации может быть в середине минуты, дня и тп - в зависимости от типа трендбара). closeDate - еобходимо чтобы было 00.00.00. Теоретически поток может вычислить startDate и уснуть - тогда если считать closeDate от начальной - то может возникнуть лаг

забыл никBlazkowiczпропущено...

Я всё равно не понял задумки.
Есть startDate и endDate. Оба вычисляются в зависимости от текущего времени. Т.е. длина не фиксирована и может быть любой?

startDate - когда трендбар реально начал мониториться( при начальной инициализации может быть в середине минуты, дня и тп - в зависимости от типа трендбара). closeDate - еобходимо чтобы было 00.00.00. Теоретически поток может вычислить startDate и уснуть - тогда если считать closeDate от начальной - то может возникнуть лаг

Хотя нет, я походу сам себя обхитрил. Вообще все неправильно имхо, в моем варианте трендбар может охватить два периода, а если считать от startDate такого не будет, но вот что с этим делать? Если startDate допустим 00:59:980, потом поток засыпает, считает closeDate через 20 миллисекунд, но ведь уже должен быть новый трендбар! Хм... надо подумать

Blazkowiczзабыл никЛинкедЛист - непотокобезопасная коллекция, а там идет ремув, апдейт из разных потоков - могут быть проблемы с visibility

В упор не вижу где это.
Есть добавление add(trendBar) в persist
Есть new LinkedList(oldList) - но он не итератор, ConcurrentModifying не выкинет, вроде.

забыл никНу и при переборе через итератор - может возникнуть исключение ConcurrentModifying.. в отличие от CopyOnWrit'a
И где он у нас этот итератор?

Да, вот я и вспомнил почему я не сделал copyOnWrite - но теперь, подумав, думаю все же зря, new LinkedList(oldList) и add(trendBar) - идут в разных потоках, теоретически второй поток может не увидеть последнего добавленного трендбара, да и вообще может не увидеть ничего на экзотических архитектурах. А итератора и правда нету(видимого), но вот вопрос а не используется ли он в конструкторе new LinkedList(oldList) - и я не уверен что все-таки не выкинет.

Blazkowiczзабыл никstartDate - когда трендбар реально начал мониториться( при начальной инициализации может быть в середине минуты, дня и тп - в зависимости от типа трендбара). closeDate - еобходимо чтобы было 00.00.00. Теоретически поток может вычислить startDate и уснуть - тогда если считать closeDate от начальной - то может возникнуть лаг
Ладно. Потом попробую воткнуть почему между
this.startDate = new Date(); и Calendar currentCalendar = Calendar.getInstance(); может быть лаг

, а между
Calendar currentCalendar = Calendar.getInstance();
и
if(getTimeUnit().equals(unit)){
уже не может быть лага.
Да, я уже сомневаюсь в имплементации, и даже пока не знаю как можно сделать правильно, подумаю на выходных:)

BlazkowiczОн там массив делает, перебирае все элементы. Попадёт или не попадёт последний элемент, не так важно. Имхо не критично.
Да, таки эксепшена не будет(посмотрел исходники).
BlazkowiczПофиг. Будем считать что слишком рано посмотрел. Посмотри позже - увидит.

Вот тут - зависит от требований приложения, мне показалось что раз финансовое приложение - то надо сделать так, чтобы все законченные трендбары были гарантированы возвращены. В веб-аппликейшене каком-нидь действительно не так бывает важно. Ну тут сугубо мое мнение.
BlazkowiczПочему?
Так по той же причине - нет ребра happens-before, компилятор и JIT могут делать все что им заблагорассудится. Нет точки синхронизации между потоками, разве я не прав?

забыл никТак по той же причине - нет ребра happens-before, компилятор и JIT могут делать все что им заблагорассудится. Нет точки синхронизации между потоками, разве я не прав?

Ну вот допустим поток, который обрабатывает quota(тот что в экзекьюторе) добавляет trendBar, но не пишет в мемори, а пишет в свой процессор store-buffer, так как нету ребра happens-before нет и указаний процессору скинуть в основную мемори, когда приходит запрос от второго потока(getTrendBars()). То есть получается что корректная работа не гарантирована, оно с высокой вероятностью будет работать, но доказать что оно БУДЕТ корректно работать нельзя