Skip to content
This repository was archived by the owner on Nov 7, 2025. It is now read-only.

Conversation

@trzysiek
Copy link
Member

@trzysiek trzysiek commented Oct 15, 2024

On old Kibana I don't see any regression with new storage on.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Nov 5, 2024

Deploying quesma with  Cloudflare Pages  Cloudflare Pages

Latest commit: 161407b
Status: ✅  Deploy successful!
Preview URL: https://4834cc4b.quesma.pages.dev
Branch Preview URL: https://persistent-storage-2.quesma.pages.dev

View logs

@trzysiek trzysiek force-pushed the persistent-storage-2 branch from 61e367a to 29679c3 Compare November 11, 2024 07:17
@trzysiek trzysiek force-pushed the persistent-storage-2 branch from 4ff28a1 to 576b195 Compare November 11, 2024 17:51
@trzysiek trzysiek force-pushed the persistent-storage-2 branch 4 times, most recently from 61b7a7d to 76035ad Compare November 11, 2024 18:05
@trzysiek trzysiek force-pushed the persistent-storage-2 branch from 76035ad to 47229e0 Compare November 11, 2024 18:05
@trzysiek trzysiek marked this pull request as ready for review December 20, 2024 16:42
@trzysiek trzysiek requested a review from a team as a code owner December 20, 2024 16:42
}

var resp *http.Response
resp, err = db.httpClient.DoRequestCheckResponseStatusOK(context.Background(), http.MethodPost, elasticsearchURL, []byte(query))
Copy link
Member

Choose a reason for hiding this comment

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

404 is OK here, but we got:

quesma-1  | Feb 10 15:06:59.942 WRN quesma/quesma/async_search_storage/in_elasticsearch.go:72 > failed to delete old documents error="response code from Elastic is not 200 OK, but 404 Not Found"

return err
}

resp, err := db.httpClient.DoRequestCheckResponseStatusOK(context.Background(), http.MethodPost, elasticsearchURL, jsonData)
Copy link
Member

Choose a reason for hiding this comment

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

resp.Body should be closed.


func (db *ElasticDatabaseWithEviction) Delete(id string) error {
elasticsearchURL := fmt.Sprintf("%s/_doc/%s", db.indexName, id)
resp, err := db.httpClient.DoRequestCheckResponseStatusOK(context.Background(), http.MethodDelete, elasticsearchURL, nil)
Copy link
Member

Choose a reason for hiding this comment

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

resp.Body should be closed

fmt.Println(query)
}

var resp *http.Response
Copy link
Member

Choose a reason for hiding this comment

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

resp.Body should be closed

}`

var resp *http.Response
resp, err = db.httpClient.DoRequestCheckResponseStatusOK(context.Background(), http.MethodGet, elasticsearchURL, []byte(query))
Copy link
Member

Choose a reason for hiding this comment

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

resp.Body should be closed

SizeInBytes() (int64, error)
SizeInBytesLimit() int64
}
JSONWithSize struct {
Copy link
Member

Choose a reason for hiding this comment

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

JSONWithSize is JSON with ID. This can be named Document


func (s AsyncRequestResultStorageInMemoryFallbackElastic) Delete(id string) {
s.inMemory.Delete(id)
go s.inElasticsearch.Delete(id)
Copy link
Member

Choose a reason for hiding this comment

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

This is risky. If there is a panic insidea s.inElasticsearch.Delete(#) whole Quesma will collapse.

We should add a recovery here:

go func() {
    recovery.LogPanic()
   s.inElasticsearch.Delete(id)
}()

This can called without a goroutine.


func (s AsyncRequestResultStorageInMemoryFallbackElastic) evict(olderThan time.Duration) {
s.inMemory.evict(olderThan)
go s.inElasticsearch.DeleteOld(olderThan)
Copy link
Member

Choose a reason for hiding this comment

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

See comment above.


func (s AsyncQueryContextStorageInMemoryFallbackElasticsearch) Store(context *AsyncQueryContext) {
s.inMemory.Store(context)
go s.inElasticsearch.Store(context)
Copy link
Member

Choose a reason for hiding this comment

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

See comment above.


func (s AsyncQueryContextStorageInMemoryFallbackElasticsearch) evict(evictOlderThan time.Duration) {
s.inMemory.evict(evictOlderThan)
go s.inElasticsearch.evict(evictOlderThan)
Copy link
Member

Choose a reason for hiding this comment

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

See comment above.

@nablaone
Copy link
Member

@trzysiek please take a look,

@trzysiek trzysiek marked this pull request as draft March 11, 2025 09:05
@pdelewski
Copy link
Contributor

/run-it

2 similar comments
@pdelewski
Copy link
Contributor

/run-it

@pdelewski
Copy link
Contributor

/run-it

@trzysiek trzysiek force-pushed the persistent-storage-2 branch from e136961 to 161407b Compare May 19, 2025 12:46
@trzysiek
Copy link
Member Author

/run-it

@pdelewski
Copy link
Contributor

I ran some tests, and unfortunately, it doesn’t work reliably.
It looks like we’re hitting an infinite loop — for example, I’m seeing this in the ecommerce dashboard.
In my opinion, it’s too risky to merge at this point.

@pdelewski pdelewski closed this May 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants