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

Add timestamps during bulk/live load #3287

Merged
merged 2 commits into from
Apr 18, 2019
Merged

Conversation

codexnull
Copy link
Contributor

@codexnull codexnull commented Apr 12, 2019

Add timestamps to make it easer to correlate load logs with server logs


This change is Reviewable

@codexnull codexnull requested a review from a team April 12, 2019 20:49
Copy link
Contributor

@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.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @codexnull)


dgraph/cmd/bulk/progress.go, line 79 at r1 (raw file):

func (p *progress) reportOnce() {
	mapEdgeCount := atomic.LoadInt64(&p.mapEdgeCount)
	timestamp := time.Now().Format("15:04:05Z0700")

Shouldn't be the timezone negative?

Also, the value is 7 during daylight savings time but is 8 the rest of the year. I would change it to 8 since that's supposed to be the "normal" time.


dgraph/cmd/live/batch.go, line 200 at r1 (raw file):

		rate := float64(counter.Nquads-last.Nquads) / period.Seconds()
		elapsed := time.Since(start).Round(time.Second)
		timestamp := time.Now().Format("15:04:05Z0700")

Same comment as in the bulk loader.

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.

I'm curious whether we can switch to glog so that timestamps are added automatically.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @codexnull)

Copy link
Contributor Author

@codexnull codexnull left a comment

Choose a reason for hiding this comment

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

I'm in favor of using glog more but I don't think it is appropriate for these messages. The glog messages include process IDs and source file names and line numbers. They are meant for the developer. These timestamps are meant for the user.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @martinmr)


dgraph/cmd/bulk/progress.go, line 79 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Shouldn't be the timezone negative?

Also, the value is 7 during daylight savings time but is 8 the rest of the year. I would change it to 8 since that's supposed to be the "normal" time.

The Z0700 is not an actual time zone. It's the place-holder for where the time zone should go when the time is formatted. And using "Z0700" instead of "-0700" just formats midnight UTC, for example, as "00:00:00Z" instead of "00:00:00-000".


dgraph/cmd/live/batch.go, line 200 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Same comment as in the bulk loader.

Done.

Copy link
Contributor

@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.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@codexnull codexnull merged commit b11cac2 into master Apr 18, 2019
@codexnull codexnull deleted the javier/load_timestamps branch April 18, 2019 21:04
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
* Add timetamp to load logs to make it easier to correlate to server logs.

* Add timezone to timestamp since client and server may be using different ones.
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.

3 participants