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

feat: store time as timespec #40

Merged
merged 4 commits into from
Jan 8, 2020
Merged

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Dec 23, 2019

To allow high resolution mtime, store time as TimeSpec message which contains seconds and nanosecond fragments.

Also allows passing mtime and mode in multiple formats, defaults mtime to the epoch, fixes coverage npm script and increases module test coverage.

Depends on:

To allow high resolution `mtime`, store time as `TimeSpec` message
which contains seconds and nanosecond fragments.

Also allows passing `mtime` and `mode`  in multiple formats, fixes
coverage npm script and increases module test coverage.
@achingbrain
Copy link
Member Author

CC @ribasushi

@alanshaw
Copy link
Member

What is TimeSpec?

@achingbrain
Copy link
Member Author

What is TimeSpec?

It's a Protobuf Message that allows us to store high resolution mtimes and will be making it into a UnixFS spec near you Soon™ - ipfs/specs#232 (comment)

Allow 0-999.. but only marshal 1-999..
Copy link

@ribasushi ribasushi left a comment

Choose a reason for hiding this comment

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

Fix ( or clarify ) nanosecond range

}

if (mtime.nsecs < 0 || mtime.nsecs > 999999999) {
throw errcode(new Error('mtime-nsecs must be within the range [0,999999999]'), 'ERR_INVALID_MTIME_NSECS')

Choose a reason for hiding this comment

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

@achingbrain what did we decide on the range? Your thumbs up at ipfs/specs#236 (comment) seems to indicate you agreed with [1,999999999]...

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR is lagging a little behind the spec discussion.

That said, I think we can reject (or omit) 0 as a nanosecond value when serializing to a protobuf but accept it at this level as a nicety to the developer.

Be liberal in what you accept and conservative in what you send, etc, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

For example, if the user passes the output of process.hrtime() as the mtime (e.g. [secs, nsecs] and it just so happens to fall exactly on the second with a 0 nanosecond fragment, it seems silly to reject the value as it creates a really subtle timing bug.

Instead we can just Do The Right Thing and omit the 0 value when writing out to a protobuf.

Choose a reason for hiding this comment

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

@achingbrain on the user-facing API side: I absolutely agree. Whenever I say "reject" I mean in the cases when a block is being decoded.

I.e. I make a distinction between "developer is constructing stuff to send to the network" and "we are decoding stuff we received from the network"

@achingbrain achingbrain merged commit 8adc245 into master Jan 8, 2020
@achingbrain achingbrain deleted the store-mtime-as-timespec branch January 8, 2020 12:49
achingbrain added a commit that referenced this pull request Feb 19, 2020
The `pending` array is emptied every iteration so just pass the
file chunk directly into the rabin chunker instead.
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