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

During startup, don't upsert initial schema if it already exists. #3374

Merged
merged 5 commits into from May 20, 2019

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented May 3, 2019

Currently, there are lots of schema insertions during startup. This
change optimizes the insertion of the schema for the initial/reserved
predicates by only doing it if the predicate is not currently being
served by any group.

Tested by starting and stoping a container and looking at the logs.
Without this change, a lot of logs about schema changes are printed
after the stopped container is restarted. With the change, the amount of
logs is greatly reduced.


This change is Reviewable

Currently, there are lots of schema insertions during startup. This
change optimizes the insertion of the schema for the initial/reserved
predicates by only doing it if the predicate is not currently being
served by any group.

Tested by starting and stoping a container and looking at the logs.
Without this change, a lot of logs about schema changes are printed
after the stopped container is restarted. With the change, the amount of
logs is greatly reduced.
@martinmr martinmr requested review from manishrjain and a team as code owners May 3, 2019 23:30
worker/groups.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @golangcibot and @manishrjain)


worker/groups.go, line 187 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not goimports-ed (from goimports)

Done.

Copy link

@gitlw gitlw left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @golangcibot and @manishrjain)

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @golangcibot and @martinmr)


worker/groups.go, line 193 at r2 (raw file):

		} else {
			// The schema for this predicate has already been proposed.
			continue

Add glog.V(1).Infof() here.

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @golangcibot and @manishrjain)


worker/groups.go, line 193 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Add glog.V(1).Infof() here.

Done.

@martinmr martinmr merged commit c92176a into master May 20, 2019
@martinmr martinmr deleted the martinmr/optimize-schema-upsert branch May 20, 2019 23:03
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
…raph-io#3374)

* During startup, don't upsert initial schema if it already exists.

Currently, there are lots of schema insertions during startup. This
change optimizes the insertion of the schema for the initial/reserved
predicates by only doing it if the predicate is not currently being
served by any group or the definition in storage is different from the
proposed schema.

Tested by starting and stoping a container and looking at the logs.
Without this change, a lot of logs about schema changes are printed
after the stopped container is restarted. With the change, the amount of
logs is greatly reduced.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants