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

notifications service #3438

Merged
merged 23 commits into from
Jun 2, 2021
Merged

notifications service #3438

merged 23 commits into from
Jun 2, 2021

Conversation

jamakase
Copy link
Contributor

@jamakase jamakase commented May 17, 2021

What

Closes #2773
Closes #2152
Closes #1495

Added
Closes #2595

How

All these 3 tasks are done as part of 1 task because they share same notifications service concept.
For show when the UI is disconnected from the API server we poll server /health every 10 seconds. After 3 unsuccessful checks - we display a notification about an error.

Includes changes that move control and some base components to separate directory components -> components/base.

Common Error concept

  • Add VersionError
  • Add ServerError
  • Change NetworkErrorBoundary to ApiErrorBoundary that is responsible for displaying common screen in case of errors.
  • Add AirbyteRequestService.parseResponse that is responsible for parsing and producing different types of errors.
  • Add views.common.ErrorOccuredView ( former ServerIsStarting component )

┆Issue is synchronized with this Asana task by Unito

@jamakase jamakase changed the title Jamakase/notifications service notifications service May 20, 2021
@jamakase jamakase marked this pull request as ready for review May 20, 2021 22:48
@auto-assign auto-assign bot requested review from cgardens and sherifnada May 20, 2021 22:48
@jamakase jamakase self-assigned this May 20, 2021
Copy link
Contributor

@cgardens cgardens left a comment

Choose a reason for hiding this comment

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

This looks great! A few questions / comments, but once you have taken a look at them, feel free to merge!

const interval = setInterval(async () => {
try {
await healthService.health();
if (count >= 3) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason we wouldn't always just unregister (even if the count is less than 3)? eh. i guess i can see why this would be wasteful.

also can you make the 3 a constant, so that if in the future we change one, we don't forget to change the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we do not need to unregister, because it won't be there yet.

Good idea to change it into constant: should it be part of config or just const above is enough? FYI interval is part of config.

try {
result = await response.json();
} catch (e) {
//
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be a comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I remember eslint does not allow empty braces, so I put comment here. May add more meaningful name

}

if (result?.error) {
if (result.error.startsWith("Version mismatch between")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to create an issue for the API to standardize the schema of its error messages. I agree with your choice here for now, but would like to have things be a bit more structured in the future, but the API has to lead on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly! Totally forgot to ask also send some sort of id for errors. But we could update it later

airbyte-webapp/src/core/request/AirbyteRequestService.ts Outdated Show resolved Hide resolved
const interval = setInterval(async () => {
try {
await healthService.health();
if (count >= 3) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we do not need to unregister, because it won't be there yet.

Good idea to change it into constant: should it be part of config or just const above is enough? FYI interval is part of config.

airbyte-webapp/src/config/index.ts Outdated Show resolved Hide resolved
try {
result = await response.json();
} catch (e) {
//
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I remember eslint does not allow empty braces, so I put comment here. May add more meaningful name

}

if (result?.error) {
if (result.error.startsWith("Version mismatch between")) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly! Totally forgot to ask also send some sort of id for errors. But we could update it later

airbyte-webapp/src/core/request/AirbyteRequestService.ts Outdated Show resolved Hide resolved
@jamakase jamakase merged commit 9af5893 into master Jun 2, 2021
@jamakase jamakase deleted the jamakase/notifications-service branch June 2, 2021 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants