-
Notifications
You must be signed in to change notification settings - Fork 0
Hw34 grpc #15
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
base: main
Are you sure you want to change the base?
Conversation
| } | ||
|
|
||
| public long getLastValue() { | ||
| return lastValue; |
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.
одного synchronized не достаточно.
Тут он тоже нужен.
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.
++
- по synchronized упустил, добавил сюда тоже
- про реализацию требования: реализовано в NumsClient через
lastServerValueв методеcalculateNextValueпроверка что одинаковое значение не использовать повторно
Если точнее - https://github.com/manfe513/java-otus/pull/15/files#diff-e3224650e63e135729d99535f01823245d730cd579fdd3aaf32b30305bb4bcd8R44
Вот на примере выдачи:
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.
не вижу в коде добавленный synchnonized
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.
не запушилось по-видимому не углядел
сейчас убрал, сделал по совету с AtomicLong
| } | ||
|
|
||
| public long getLastValue() { | ||
| return lastValue; |
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.
не вижу в коде добавленный synchnonized
|
|
||
| long nextVal = currentValue; | ||
|
|
||
| if (serverValue != lastServerValue && serverValue > 0) { |
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.
это условие
serverValue > 0
кажется тут лишним.
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.
++
|
|
||
| long nextVal = currentValue; | ||
|
|
||
| if (serverValue != lastServerValue && serverValue > 0) { |
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.
сравнивать значение с предыдущим не очень надежно.
Т.к. значение может задвоиться и его пропустите.
Надежнее и проще использовать AtomicLong.getAndSet(0)
тогда и synchronized будет не нужен и логика заметно упростится.
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.
убрал synchronized
сделал с AtomicLong
| long serverValue = streamObserver.getLastValue(); | ||
| logger.info("serverValue: {}", currentValue); | ||
|
|
||
| long previousServerValue = lastServerValue.getAndSet(serverValue); |
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.
тут проще можно:
currentValue + lastServerValue.getAndSet(0) + 1;
No description provided.