Use one atomic variable to generate blank node ids for json objects

[gitlw]

This change is 

[pullrequest]

✅ A review job has been created and sent to the PullRequest network.

---

@gitlw you can click here to see the review status or cancel the code review job.

[pullrequest]

	1 Message
✅	The description of this pull request is blank. Adding a high-level summary will help our reviewers provide better feedback.

[manishrjain]

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gitlw)

---

chunker/json_parser.go, line 277 at r1 (raw file):

// GetNextIdx is only used for testing
func getNextIdx() uint64 {

getNextBlank() return string. Also, at process start, generate a random int of 4 bytes. And the total string should be:

_:dg.%d.%d. The first int is the process level int. The second is the idx by atomic.

---

chunker/json_parser.go, line 278 at r1 (raw file):

// GetNextIdx is only used for testing
func getNextIdx() uint64 {
	return atomic.LoadUint64(&nextIdx)

return atomic.AddUint64(&nextIdx, 1)

---

chunker/json_parser.go, line 326 at r1 (raw file):

		idx := atomic.LoadUint64(&nextIdx)
		atomic.AddUint64(&nextIdx, 1)

No need for this.

---

chunker/json_parser.go, line 328 at r1 (raw file):

		atomic.AddUint64(&nextIdx, 1)

		mr.uid = fmt.Sprintf("_:blank-%d", idx)

Directly use getNextIdx() here.

---

chunker/json_parser_test.go, line 145 at r1 (raw file):

	require.Equal(t, 6, len(nq))

	aliceId := fmt.Sprintf("_:blank-%d", baseIdx)

aliceId := getNextBlank()

[pullrequest]

Have a question primarily on the use of the atomic add and a couple other minor suggestions.

---

Reviewed with ❤️ by PullRequest

[martinmr]

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @gitlw)

---

chunker/json_parser.go, line 273 at r2 (raw file):

// nextIdx is the index that is used to generate blank node ids for a json map object
// when the map object does not have a "uid" field

minor: period.

---

chunker/json_parser.go, line 274 at r2 (raw file):

// nextIdx is the index that is used to generate blank node ids for a json map object
// when the map object does not have a "uid" field
// it should only be accessed through the atomic APIs

minor: period and uppercase.

---

chunker/json_parser.go, line 277 at r2 (raw file):

var nextIdx uint64

// randomId will be used to generate blank node ids

minor: period and uppercase.

---

chunker/json_parser_test.go, line 95 at r2 (raw file):

}

type Experiment struct {

maybe call this Test?

[manishrjain]

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @gitlw and @pullrequest[bot])

---

chunker/json_parser.go, line 326 at r1 (raw file):

Previously, pullrequest[bot] wrote…

Is it possible that between the LoadUint64 and the AddUint64 here that another AddUint64 has already come in on another thread/goroutine? This might lead to some cases having duplicate idx values used across concurrent access, unless that is okay?

It looks like AddUint64 returns the new value so you could potentially drop the LoadUint64 altogether and just use the new returned value to infer idx at the time of the increment. Eg:

idx := atomic.AddUint64(&nextIdx, 1) - 1

Nice catch. I had the same comment.

---

chunker/json_parser_test.go, line 84 at r2 (raw file):

	ctx := context.Background()
	require.NoError(exp.t, dg.Alter(ctx, &api.Operation{DropAll: true}), "drop all failed")
	require.NoError(exp.t, dg.Alter(ctx, &api.Operation{Schema: exp.schema}), "schema change failed")

100 chars.