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

Issue 1712 add status information #1747

Merged
merged 34 commits into from
Oct 4, 2023

Conversation

aindriu-aiven
Copy link
Contributor

@aindriu-aiven aindriu-aiven commented Sep 8, 2023

About this change - What it does
This PR adds a lastStatusCheckTimestamp

This PR refactors environment caching reducing the number of caches and altering how they communicate so that they do not require a full cache reset on every change to an environment.

There is a second PR that will remove a further cache and also allow the use of Kafka to synchronize multiple instances if desired.

The application.properties has three additional properties

#This allows App2App comunication in a HA setup with multiple Klaw-core instances

# Provide a base 64 encoded string. The same secret should be used in all Instances.

klaw.core.app2app.base64.secret=
A username that can be altered to suit the users needs and improve the security of the app2app user.
klaw.core.app2app.username=KlawApp2App

# Enable High Availability for multi-instance Klaw-core deployment.

# If set to false the additional APIs required for HA will not be exposed reducing resource usage and improving security.

klaw.core.ha.enable=true
Resolves: #1712
Why this way

…m db every time an env is changed

Signed-off-by: Aindriu Lavelle <[email protected]>
…m db every time an env is changed

Signed-off-by: Aindriu Lavelle <[email protected]>
Signed-off-by: Aindriu Lavelle <[email protected]>
Signed-off-by: Aindriu Lavelle <[email protected]>
Signed-off-by: Aindriu Lavelle <[email protected]>
@aindriu-aiven aindriu-aiven marked this pull request as ready for review September 8, 2023 13:34
@aindriu-aiven
Copy link
Contributor Author

Screenshot with envStatusTime returned
image

@@ -59,6 +60,7 @@ const mockedEnvironmentsResponse: EnvironmentPaginatedApiResponse =
tenantName: "default",
clusterName: "DEV_CLS",
envStatus: "ONLINE",
envStatusTime: "2023-09-21T11:47:15.664615239",
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 wondering if we don't also want a requesttimestring property as we have for the[entity]RequestsResponseModel. We can parse it on the front end, but then we have a potential for divergence if the formatting changes in one of the two spots we are now doing time formatting in :o

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 am adding in entStatusRequestedAtTime

Copy link
Contributor

@mathieu-anderson mathieu-anderson Sep 26, 2023

Choose a reason for hiding this comment

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

I'm sorry, I didn't mean to add the time of the request, but having the envStatusTime formatted in the same way as requesttimestring 🙇 Maybe the following is already what is happening, but the test values express a different thing: https://github.com/Aiven-Open/klaw/pull/1747/files#diff-df59e5cc730be0ccaca740fec7b48fdf327c4430601001cf99da3813e39f32d7R42

The difference between these two: https://github.com/Aiven-Open/klaw/blob/main/openapi.yaml#L7062-L7068

Is that one is the undformatted timestamp (requesttime) and the other is a formatted string expressing the time in the UTC tz (requesttimestring)

We are currently doing no parsing of timestamps in the FE, only rendering the already formatted requesttimestring:

Screenshot 2023-09-26 at 13 33 28

And I thought we might do the same for the envStatusTime timestamp: same time, but formatted differently.

Copy link
Contributor

@muralibasani muralibasani left a comment

Choose a reason for hiding this comment

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

LGTM, thanks,

@aindriu-aiven aindriu-aiven merged commit 28dbbd5 into main Oct 4, 2023
21 checks passed
@aindriu-aiven aindriu-aiven deleted the issue-1712-add-status-information branch October 4, 2023 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants