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

Comments and more improvement. #3

Merged
merged 6 commits into from
Sep 12, 2014
Merged

Comments and more improvement. #3

merged 6 commits into from
Sep 12, 2014

Conversation

tallytalwar
Copy link
Member

more logic to not do extra processing. A lot more comments to explain whats happening.

@@ -129,8 +143,8 @@ bool MapzenVectorTileJson::LoadTile(std::vector<glm::ivec3> tileCoords) {

//timeout specification for select() call
//select() will unblock either when a fd is ready or tileout is reached
timeout.tv_sec = 0;
timeout.tv_usec = 100;
timeout.tv_sec = 1; //enough time for fd to be ready reading data... could be optimized.
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean it waits 1 second? Most tiles should be done well before then (if they are performing well...).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya. so select will either halt the thread for 1 second or continue if a file descriptor is ready.

So worst case if no tile url is responding it will timeout in 1 second... and try again .. I can remove the try again portion and return false ...

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, so this is a timeOUT? If it hits the timeout and needs to retry, is there a retry limit? (3 is a good default rule of thumb)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya right now it will keep on trying.. I will put a limit on that.

Which reminds me ... we should have a header with all such "MAGIC" number const/#define macros defined. What say @blair1618 and @patriciogonzalezvivo

@tallytalwar
Copy link
Member Author

Updated

@tallytalwar tallytalwar reopened this Sep 10, 2014
return false;
}

//wait and repeat until curl has something to report to the kernel wrt file descriptors
// TODO: if no internet, then this gets stuck... put a timeout here.
Copy link
Member Author

Choose a reason for hiding this comment

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

added a very basic logic to timeout if fdset never sets a valid fdmax (can happen with super slow internet or no internet or service down)

Copy link
Member

Choose a reason for hiding this comment

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

Cool - connectivity is something we will want to think carefully about across the library. Both on mobile generally and in NYC specifically, uneven connectivity is a common issue (bad coverage, subway, etc.). Also just requires a lot of real-world testing.

…ync between the producer (tile data being generated) and consumer (tiles being rendered). Can work on a producer-consumer sync when the time is right.
…ync between the producer (tile data being generated) and consumer (tiles being rendered). Can work on a producer-consumer sync when the time is right.
tallytalwar added a commit that referenced this pull request Sep 12, 2014
1. Implements libCurl multi interface with select() and file descriptor setting and error checking.
2. Implements relevant timeout:
    2a. Timeout for not able to contact the service
    2b. Timeout for not able to retrieve the data in 1 sec from the service.
3. Implements simple std::thread for fetching of tiles from service.
4. Implements CheckDataExists for DataSource class.
@tallytalwar tallytalwar merged commit de18661 into master Sep 12, 2014
@tallytalwar tallytalwar deleted the curl-async-fetching branch September 12, 2014 18:51
@JunsuLime JunsuLime mentioned this pull request Jun 16, 2017
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.

2 participants