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.

Sign up for GitHub

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

Jump to bottom

Fix Go and Java SDK Regressions #729

Merged
merged 5 commits into from
May 29, 2020
Merged

Fix Go and Java SDK Regressions #729

merged 5 commits into from
May 29, 2020

Conversation

mrzzy
Copy link
Collaborator

@mrzzy mrzzy commented May 22, 2020 •
edited
Loading

What this PR does / why we need it:
This is a split off containing solely the bug fixes of PR #720

Fixes SDK bugs introduced in PR #693 & #700

Does this PR introduce a user-facing change?:

Fixed Go SDK error due to trying to add to a nil map.
Fixed Go SDK omitted entity values when get online features.
Fixed Java SDK's Row.set() does not accept feast Value type

@mrzzy mrzzy changed the title Fix sdk regressions Fix Go and Java SDK Regressions May 22, 2020
@ches
Copy link
Member

ches commented May 22, 2020 •
edited
Loading

This is a split off containing solely the bug fixes of PR #725

Is the correct issue number reference? I'm not finding where these come from and it definitely doesn't look like #725 ☺️

Edit: Should be #720, took the liberty of updating the PR description myself.

@ches ches added backport-candidate Changes that may be desired for backport to earlier Feast release tracks kind/bug labels May 22, 2020
@ches
Copy link
Member

ches commented May 22, 2020

Does this PR introduce a user-facing change?:

NONE

Bug fixes should definitely have a changelog IMO, although I guess we currently use PR titles rather than this field. The PR number references where issues were introduced are super helpful, the extra mile would be if we also note the Feast version number where a bug was introduced, if it has been released yet. Good for users to quickly know if they're affected, good for us to know where to backport.

@woop
Copy link
Member

woop commented May 23, 2020

Does this PR introduce a user-facing change?:

NONE

Bug fixes should definitely have a changelog IMO, although I guess we currently use PR titles rather than this field. The PR number references where issues were introduced are super helpful, the extra mile would be if we also note the Feast version number where a bug was introduced, if it has been released yet. Good for users to quickly know if they're affected, good for us to know where to backport.

Yea we are using the PR titles right now. Ideal case would be to use the release note if available, and fall back to the PR. I dont think our changelog generator supports that right now though. So we might need to update our release process.

By the way, we can enforce release notes using a Prow plugin as well: http://prow.feast.ai/plugins

@mrzzy mrzzy requested a review from pyalex as a code owner May 26, 2020 03:52
@woop
Copy link
Member

woop commented May 28, 2020

@mrzzy mind updating the release note so that I can merge this?

@woop
Copy link
Member

woop commented May 29, 2020

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrzzy, woop

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

@feast-ci-bot feast-ci-bot merged commit 714b0c2 into feast-dev:master May 29, 2020
khorshuheng pushed a commit to khorshuheng/feast that referenced this pull request Jun 5, 2020
@mrzzy @khorshuheng
Fix Go and Java SDK Regressions (feast-dev#729)
b79274f 
* Fix nil error cause by  adding items to a nil map in go SDK's client.GetOnlineFeatures()

* Fix issue where go sdk client.GetOnlineFeatures() does not return entity values

* Fixed issue where Row is unable to parse Value type due to proto package rename.

* Fix wrong path of test_feature python SDK test.

* Revert stub and channel private fields to final in FeastClient java SDK

Co-authored-by: Zhu Zhanyan <zhu.zhanyan@gojek.com>
@khorshuheng khorshuheng mentioned this pull request Jun 5, 2020
Merged
khorshuheng pushed a commit that referenced this pull request Jun 5, 2020
@mrzzy @khorshuheng
Fix Go and Java SDK Regressions (#729)
a4adc25 
* Fix nil error cause by  adding items to a nil map in go SDK's client.GetOnlineFeatures()

* Fix issue where go sdk client.GetOnlineFeatures() does not return entity values

* Fixed issue where Row is unable to parse Value type due to proto package rename.

* Fix wrong path of test_feature python SDK test.

* Revert stub and channel private fields to final in FeastClient java SDK

Co-authored-by: Zhu Zhanyan <zhu.zhanyan@gojek.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woop woop woop approved these changes

@davidheryanto davidheryanto Awaiting requested review from davidheryanto

@khorshuheng khorshuheng Awaiting requested review from khorshuheng

@pradithya pradithya Awaiting requested review from pradithya

@zhilingc zhilingc Awaiting requested review from zhilingc

@pyalex pyalex Awaiting requested review from pyalex

Assignees

@woop woop

Labels
approved backport-candidate Changes that may be desired for backport to earlier Feast release tracks kind/bug lgtm size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mrzzy @ches @woop @feast-ci-bot