Skip to content
This repository has been archived by the owner on Aug 28, 2021. It is now read-only.

NBS: Remove reverse-order hack in httpBatchStore #2982

Closed
cmasone-attic opened this issue Dec 23, 2016 · 2 comments
Closed

NBS: Remove reverse-order hack in httpBatchStore #2982

cmasone-attic opened this issue Dec 23, 2016 · 2 comments
Labels

Comments

@cmasone-attic
Copy link
Contributor

In order to allow value writing to send chunks to the server in write-order, I had to take out the code that sorted chunks-to-be-written by ref-height before sending them to the server. Unfortunately, Pull() relies on functionality like this to allow it to process and SchedulePut() chunks in a top-down fashion. There's now a hack that causes httpBatchStore.Flush() to send chunks in reverse-insert-order.

Stop this. Do a good thing instead :-)
@fyi rafael-atticlabs

cmasone-attic added a commit to cmasone-attic/noms that referenced this issue Dec 23, 2016
NBS benefits from related chunks being near one another. Initially,
let's use write-order as a proxy for "related".

This patch contains a pretty heinous hack to allow sync to continue
putting chunks into httpBatchStore top-down without breaking
server-side validation. Work to fix this is tracked in attic-labs#2982

This patch fixes attic-labs#2968, at least for now
cmasone-attic added a commit to cmasone-attic/noms that referenced this issue Dec 23, 2016
NBS benefits from related chunks being near one another. Initially,
let's use write-order as a proxy for "related".

This patch contains a pretty heinous hack to allow sync to continue
putting chunks into httpBatchStore top-down without breaking
server-side validation. Work to fix this is tracked in attic-labs#2982

This patch fixes attic-labs#2968, at least for now
@ghost
Copy link

ghost commented Dec 23, 2016

I actually think that this "hack" is a decent half-step towards "the right thing". Sending in reverse order is convenient because it's an in-order scan.

If we could modify sync so that each height level is written in "chunk" order, then just sending chunks in reverse insertion order might actually create optimal locality (i.e. not just parent-child, but sibling).

A (later) step three might be to allow "writes" to take place in this order (i.e. top-down, but in correct "chunk" order), so as to avoid the temporary buffering step).

cmasone-attic added a commit that referenced this issue Dec 23, 2016
…2983)

NBS benefits from related chunks being near one another. Initially,
let's use write-order as a proxy for "related".

This patch contains a pretty heinous hack to allow sync to continue
putting chunks into httpBatchStore top-down without breaking
server-side validation. Work to fix this is tracked in #2982

This patch fixes #2968, at least for now

* Introduces PullWithFlush() to allow noms sync to explicitly
pull chunks over and flush directly after. This allows UpdateRoot
to behave as before.

Also clears out all the legacy batch-put machinery. Now, Flush()
just directly calls sendWriteRequests().
@ghost ghost added the P0 label Feb 11, 2017
@ghost ghost changed the title NBS: Address reverse-order hack in httpBatchStore NBS: Remove reverse-order hack in httpBatchStore Feb 11, 2017
@ghost
Copy link

ghost commented Feb 11, 2017

Blocked on #3178

cmasone-attic added a commit to cmasone-attic/noms that referenced this issue Apr 5, 2017
With our old validation strategy, it was critical that
chunk graphs be written bottom-up, during both novel value
creation and sync. With the strategy implemented in attic-labs#3180,
this is no longer required, which lets us get rid of a bunch
of machinery:

1) The reverse-order hack in httpBatchStore
2) the EnumerationOrder stuff in NomsBlockCache
3) the orderedPutCache in datas/
4) the refHeight arg on SchedulePut()

Fixes attic-labs#2982
cmasone-attic added a commit to cmasone-attic/noms that referenced this issue Apr 6, 2017
With our old validation strategy, it was critical that
chunk graphs be written bottom-up, during both novel value
creation and sync. With the strategy implemented in attic-labs#3180,
this is no longer required, which lets us get rid of a bunch
of machinery:

1) The reverse-order hack in httpBatchStore
2) the EnumerationOrder stuff in NomsBlockCache
3) the orderedPutCache in datas/
4) the refHeight arg on SchedulePut()

Fixes attic-labs#2982
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

1 participant