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 sideload node and service #1637

Merged
merged 1 commit into from
Nov 8, 2017
Merged

Add sideload node and service #1637

merged 1 commit into from
Nov 8, 2017

Conversation

nathanielc
Copy link
Contributor

@nathanielc nathanielc commented Oct 24, 2017

Adds a sideload node that acts similar to the default node but allows the data to be loaded from external sources.

TODO

  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Trigger source update over API or signal
  • Check only files below the source can be used.
  • Cache source objects so they are reused per unique source
  • Get feedback on {tag} syntax.
  • Figure out what to do about conflicting ToRow usage.
  • Update API docs
  • Update CLI to trigger a sideload reload (Decided not to do this)

@ghost ghost assigned nathanielc Oct 24, 2017
@ghost ghost added the in progress label Oct 24, 2017
@nathanielc nathanielc requested a review from desa October 25, 2017 21:34
Copy link
Contributor

@desa desa left a comment

Choose a reason for hiding this comment

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

Just a couple minor things. 💯

sideload.go Outdated
bufferPool *bufpool.Pool
}

// Create a new SideloadNode which shifts points and batches in time.
Copy link
Contributor

Choose a reason for hiding this comment

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

which shifts points and batches in time.

Is this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope,:)

sideload.go Outdated
order: make([]string, len(n.OrderList)),
orderTmpls: make([]orderTmpl, len(n.OrderList)),
}
u, err := url.Parse(n.Source)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should lines 40-49 go into the et.tm.SideloadService.Source method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that would make more sense.

}
n.order[i] = p
}
if len(n.s.Fields) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sucks that the logic for fields and tags can't be pulled into a common method without adding reflection.

Copy link
Contributor

Choose a reason for hiding this comment

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

:(

@nathanielc nathanielc changed the title [WIP] Add sideload node and service Add sideload node and service Nov 8, 2017
@nathanielc nathanielc merged commit 216b381 into master Nov 8, 2017
nathanielc added a commit that referenced this pull request Nov 8, 2017
@ghost ghost removed the in progress label Nov 8, 2017
@nathanielc nathanielc deleted the nc-sideload branch November 8, 2017 16:51
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