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

Change error store to be part of configuration instead #39

Merged
merged 9 commits into from
Jan 9, 2019

Conversation

zhilingc
Copy link
Collaborator

@zhilingc zhilingc commented Jan 5, 2019

Fixes #27.

This change requires core to be deployed with the following additional configuration:

  • feast.jobs.errorsStoreType
  • feast.jobs.errorsStoreOptions

which define where (and how) to write the error logs of each ingestion job. These options are passed in as flags to the ingestion jar when a job is run. Internally to ingestion a storage spec is still used.

@zhilingc zhilingc requested a review from tims January 5, 2019 01:45
@tims
Copy link
Contributor

tims commented Jan 6, 2019

We could change the [Error|Serving|Warehouse]Store classes to have factory in the name if that makes them make more sense. I just found it very verbose.

@zhilingc
Copy link
Collaborator Author

zhilingc commented Jan 8, 2019

/retest

@zhilingc
Copy link
Collaborator Author

zhilingc commented Jan 8, 2019

/retest

@zhilingc
Copy link
Collaborator Author

zhilingc commented Jan 8, 2019

@tims

@@ -38,7 +39,7 @@

public static List<ErrorsStore> getAll() {
return Lists.newArrayList(
Iterators.concat(manuallyRegistered.iterator(), serviceLoader.iterator()));
Iterators.concat(manuallyRegistered.iterator(), serviceLoader.iterator()));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is weird, are you using the google-java-format plugin or the imported googles styles xml?
https://github.com/google/styleguide/blob/gh-pages/intellij-java-google-style.xml

The imports don't get rearranged for me either.

@tims
Copy link
Contributor

tims commented Jan 9, 2019

Looks good except for formatting and import org, which need the google styles applied.
i.e: https://google.github.io/styleguide/javaguide.html#s3.3.3-import-ordering-and-spacing

If the google-java-format plugin neglects to org imports correctly. So instead, import the intellij styles.
https://github.com/google/styleguide/blob/gh-pages/intellij-java-google-style.xml

@tims
Copy link
Contributor

tims commented Jan 9, 2019

We should definitely merge this one before I merge this one before PR #52

@zhilingc
Copy link
Collaborator Author

zhilingc commented Jan 9, 2019

/assign pradithya

@tims
Copy link
Contributor

tims commented Jan 9, 2019

/approve

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tims

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@pradithya
Copy link
Collaborator

/lgtm

@feast-ci-bot feast-ci-bot merged commit 5ac4b9c into feast-dev:master Jan 9, 2019
@zhilingc zhilingc deleted the error-store-fix branch January 14, 2019 09:42
Yanson pushed a commit to Yanson/feast that referenced this pull request Jul 29, 2020
dmartinol added a commit to dmartinol/feast that referenced this pull request Jul 10, 2024
Adding registry server (UT to be completed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants