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

Revendor Cortex #2755

Merged
merged 8 commits into from
Oct 14, 2020
Merged

Revendor Cortex #2755

merged 8 commits into from
Oct 14, 2020

Conversation

owen-d
Copy link
Member

@owen-d owen-d commented Oct 13, 2020

In addition to revendoring Cortex, this includes

  • various compatibility changes
  • a change to the object store interface used in the boltdb shipper (upstream compat)
  • refactors Prometheus service discovery for upstream compatibility.

This update will include cortexproject/cortex#3314 (a fix for the sharded ruler).

/cc @slim-bean @sandeepsukhani

Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

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

LGTM @sandeepsukhani can you take a look at the delimeter changes? I think we talked about a method on the stores for asking for the delimeter so we don't have to hard code it, not sure where that ended up.

Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

Some minor fixes to be taken care of otherwise it LGTM.

@@ -61,7 +61,7 @@ func newTable(ctx context.Context, workingDirectory string, objectClient chunk.O
}

func (t *table) compact() error {
objects, _, err := t.storageClient.List(t.ctx, t.name+"/")
Copy link
Contributor

Choose a reason for hiding this comment

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

The forward slash here needs to stay because we are trying to list contents of a directory without it we will get the name of the same directory back with hosted object stores. This is due to the object stores not having a concept of directories.

@@ -120,7 +121,7 @@ func (t *Table) init(ctx context.Context, spanLogger *spanlogger.SpanLogger) (er
startTime := time.Now()
totalFilesSize := int64(0)

objects, _, err := t.storageClient.List(ctx, t.name+"/")
Copy link
Contributor

Choose a reason for hiding this comment

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

same suggestion here as well.

@@ -308,7 +309,7 @@ func (t *Table) Sync(ctx context.Context) error {
func (t *Table) checkStorageForUpdates(ctx context.Context) (toDownload []chunk.StorageObject, toDelete []string, err error) {
// listing tables from store
var objects []chunk.StorageObject
objects, _, err = t.storageClient.List(ctx, t.name+"/")
Copy link
Contributor

Choose a reason for hiding this comment

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

same here.

@sandeepsukhani
Copy link
Contributor

LGTM @sandeepsukhani can you take a look at the delimeter changes? I think we talked about a method on the stores for asking for the delimeter so we don't have to hard code it, not sure where that ended up.

@slim-bean Cortex team has changed the code to convert delimiter on windows between request/response. We just have to pass paths with "/" always and they have used https://golang.org/pkg/path/filepath/#FromSlash and https://golang.org/pkg/path/filepath/#ToSlash functions which comes from standard go package so we don't have to worry about it.

Copy link
Contributor

@sandeepsukhani sandeepsukhani 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 for taking care of it!

@owen-d owen-d merged commit 54c0c5f into grafana:master Oct 14, 2020
cyriltovena pushed a commit to cyriltovena/loki that referenced this pull request Oct 21, 2020
* replaces prometheus & revendors cortex

* compat

* compat

* sd refactor

* updates docker

* removes prom replace

* adds delimiters back to object addrs in boltdb

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants