Skip to content

Conversation

@scbizu
Copy link
Contributor

@scbizu scbizu commented May 16, 2021

The patch includes:

But still need some more load-testing ...

The patch includes:

* increases most of the museum api throughout by wrapping the event
  emitter into goroutine since the event listener already holds on the
  concurrent lock(#396).

* adds the new server option `keep-chart-always-up-to-date` to force use
  the latest version.

* `getIndexFile` rollbacks to fully reload index only if cacheinterval
  not set for better backward compatibility before v0.13.0(#444).

Signed-off-by: scnace <scbizu@gmail.com>
@scbizu scbizu force-pushed the perf/increase-api-server-throughout branch from 92bf1eb to d70bb01 Compare May 16, 2021 09:51
@scbizu scbizu added this to the v0.13.2 milestone May 16, 2021
@scbizu scbizu changed the title [WIP] Work on increasing the server throughout. work on increasing the server throughout. May 16, 2021
@scbizu scbizu requested a review from jdolitsky May 16, 2021 17:24
@scbizu scbizu changed the title work on increasing the server throughout. proposal: work on increasing the server throughout. May 22, 2021
Comment on lines +360 to +364
if server.KeepChartAlwaysUpToDate {
// waiting for the index regeneration and keep the entry index always up-to-date
server.Tenants[repo].RegenerationLock.Lock()
defer server.Tenants[repo].RegenerationLock.Unlock()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@scbizu - can you explain this a bit? I thought KeepChartAlwaysUpToDate would use the old logic prior to adding emitEvent stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To rollback the old logic we may not need to add the new option KeepChartAlwaysUpToDate , we only need to check whether the cache-interval is set or not , and we will use the old logic if cache-interval not set , otherwise , set the provided cache interval, and enables the event emitter.

My original purpose adding the new option KeepChartAlwaysUpToDate is that if we can have some mechanisms to always get the latest chart with the event emitter is enabled.

@jdolitsky
Copy link
Contributor

@scbizu can you rebase?

@scbizu scbizu marked this pull request as draft January 28, 2022 16:33
@scbizu
Copy link
Contributor Author

scbizu commented Jan 28, 2022

This pr is not fully tested , maybe should be delayed into v0.14.1 or later version. I convert it to draft for now .

@scbizu scbizu removed this from the v0.14.0 milestone Jan 28, 2022
@scbizu scbizu self-assigned this Jan 28, 2022
@scbizu scbizu added this to the v0.15.0 milestone Jan 28, 2022
@cbuto
Copy link
Contributor

cbuto commented May 31, 2022

@scbizu after #396 was closed, do we need to still fix up this PR?

@scbizu
Copy link
Contributor Author

scbizu commented May 31, 2022

I want to close this PR , and cherry pick some commits out (such as the fix for #498 ).

BTW I really forget this PR , thanks for reminding me of this .

@scbizu scbizu closed this Jun 5, 2022
@scbizu
Copy link
Contributor Author

scbizu commented Jun 5, 2022

Moved to #593

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants