Skip to content

Change file system FileExists interface to return Status - #5370

Closed
llhe wants to merge 1 commit into
tensorflow:masterfrom
llhe:fileexists2
Closed

Change file system FileExists interface to return Status#5370
llhe wants to merge 1 commit into
tensorflow:masterfrom
llhe:fileexists2

Conversation

@llhe

@llhe llhe commented Nov 3, 2016

Copy link
Copy Markdown
Contributor

The pull request addresses issue #5321.

The calling code are modified to handle error condition at best effort, if the outer caller function does not return error, it's simply changed to

FileExists(name, &result).ok() && result

Tests:
Tests run except windows implementation. HDFS is tested with 2.7.3 pseudo-distributed mode.

PS: The folowwing tests may fail when all tests run together, but pass when run standalone, should not related to this change

//tensorflow/contrib/session_bundle:exporter_test
//tensorflow/tensorboard/backend:server_test
//tensorflow/contrib/tensor_forest/hybrid:k_feature_routing_function_op_test
//tensorflow/contrib/learn:random_forest_test

@tensorflow-jenkins

tensorflow-jenkins commented Nov 3, 2016

Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

@mention-bot

mention-bot commented Nov 3, 2016

Copy link
Copy Markdown

@llhe, thanks for your PR! By analyzing the history of the files in this pull request, we identified @keveman, @rinugun and @caisq to be potential reviewers.

@vrv

vrv commented Nov 3, 2016

Copy link
Copy Markdown

Thanks for making this change! It's going to be easier for us to make this change internally, since it changes a core public interface that a lot of people are using internally.

cc @jhseu @rohan100jain, perhaps use this change as a seed for the internal change, and make sure to credit @llhe ?

Alternatively, we can keep the old interface around so we can pull this change internally without too many problems, and then get rid of it once all of the internal uses are changed.

@jhseu

jhseu commented Nov 3, 2016

Copy link
Copy Markdown
Contributor

I actually already have an equivalent change internally that I was going to send out for review today. There are quite a few call sites internally, so it's easier to make the change there.

Note that I'm going to change the signature to:
Status FileExists(const string& fname); where it returns NOT_FOUND when it doesn't exist. That matches some existing conventions that we're using.

@vrv

vrv commented Nov 3, 2016

Copy link
Copy Markdown

Okay, cool, I will close this and we'll have the internal fix pushed out soon. Thanks @llhe !

@vrv vrv closed this Nov 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@rohan100jain rohan100jain

@jhseu jhseu

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@llhe @tensorflow-jenkins @mention-bot @vrv @jhseu @rohan100jain @googlebot