Skip to content

PLAT-653: ensure "try" methods of fs utilities do not throw during commo... #127

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

Open
wsourdeau wants to merge 5 commits into
base: master
Choose a base branch
from
Open

PLAT-653: ensure "try" methods of fs utilities do not throw during commo... #127

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
370fc55
PLAT-653: ensure "try" methods of fs utilities do not throw during co…
wsourdeau Sep 5, 2014
34ca7c7
Merge branch 'master' into PLAT-653_fs-utils-try-not-throw
Sep 23, 2014
54844b7
PLAT-653: removed tryGetInfo from UrlFsHandler interface
Sep 23, 2014
147b397
adjust declaration and documentation
Sep 23, 2014
0641c33
Merge branch 'master' into PLAT-653_fs-utils-try-not-throw
Nov 25, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 17 additions & 15 deletions service/fs_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,20 +62,27 @@ static FsObjectInfo extractInfo(const struct stat & stats)

struct LocalUrlFsHandler : public UrlFsHandler {

virtual FsObjectInfo getInfo(const Url & url) const
virtual FsObjectInfo getInfo(const Url & url)
const
{
FsObjectInfo info;

struct stat stats;
string path = url.path();

// cerr << "fs info on path: " + path + "\n";
int res = ::stat(path.c_str(), &stats);
if (res == -1) {
throw ML::Exception(errno, "stat");
if (res == 0) {
info = extractInfo(stats);
}
else {
if (errno != ENOENT) {
throw ML::Exception(errno, "stat");
}
}

// TODO: owner ID (uid) and name (uname)

return extractInfo(stats);
return info;
}

virtual void makeDirectory(const Url & url) const
Expand Down Expand Up @@ -196,9 +203,10 @@ namespace Datacratic {

/* URLFSHANDLER */

size_t
int64_t
UrlFsHandler::
getSize(const Url & url) const
getSize(const Url & url)
const
{
return getInfo(url).size;
}
Expand Down Expand Up @@ -238,16 +246,10 @@ getUriObjectInfo(const std::string & url)
FsObjectInfo
tryGetUriObjectInfo(const std::string & url)
{
JML_TRACE_EXCEPTIONS(false);
try {
return getUriObjectInfo(url);
}
catch (...) {
return FsObjectInfo();
}
return getUriObjectInfo(url);
}

size_t
int64_t
getUriSize(const std::string & url)
{
Url realUrl = makeUrl(url);
Expand Down
8 changes: 4 additions & 4 deletions service/fs_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ OnUriObject;
struct UrlFsHandler {
virtual FsObjectInfo getInfo(const Url & url) const = 0;

virtual size_t getSize(const Url & url) const;
virtual int64_t getSize(const Url & url) const;
virtual std::string getEtag(const Url & url) const;

virtual void makeDirectory(const Url & url) const = 0;
Expand Down Expand Up @@ -101,11 +101,11 @@ void registerUrlFsHandler(const std::string & scheme,
FsObjectInfo getUriObjectInfo(const std::string & filename);

// Return the object info for either a file or an S3 object, or null if
// it doesn't exist
FsObjectInfo tryGetUriObjectInfo(const std::string & filename);
// it doesn't exist (deprecated, use getUriObjectInfo instead)
FsObjectInfo tryGetUriObjectInfo(const std::string & filename) __attribute__((__deprecated__));
Copy link
Contributor

@jeremybarnes jeremybarnes Sep 5, 2014

Choose a reason for hiding this comment

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

Why is this deprecated? I use it all over the place. It will stop lots of code from compiling.

Copy link
Contributor Author

@wsourdeau wsourdeau Sep 8, 2014

Choose a reason for hiding this comment

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

I thought this would only generate a warning. The deprecation, because both methods are now using the same code, and my intention was for it to serve only as an indicator for new code.

Copy link
Contributor Author

@wsourdeau wsourdeau Sep 10, 2014

Choose a reason for hiding this comment

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

@jeremybarnes does getUriObjectInfo need to throw if the object is not found? Is it considered part of the semantics?

Copy link
Contributor

@jeremybarnes jeremybarnes Sep 23, 2014

Choose a reason for hiding this comment

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

yes, it is an important part of the semantics. It allows you to chain calls without regard for checking the return code for each one.


// Return an URI for either a file or an s3 object
size_t getUriSize(const std::string & filename);
int64_t getUriSize(const std::string & filename);

// Return an etag for either a file or an s3 object
std::string getUriEtag(const std::string & filename);
Expand Down
2 changes: 1 addition & 1 deletion service/s3.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ struct S3UrlFsHandler : public UrlFsHandler {
{
string bucket = url.host();
auto api = getS3ApiForBucket(bucket);
return api->getObjectInfo(bucket, url.path().substr(1));
return api->tryGetObjectInfo(bucket, url.path().substr(1));
}

virtual void makeDirectory(const Url & url) const
Expand Down