This is a workable version based on original PDC.get_latest_modules without tests updated. There is conflicts with PR https://pagure.io/freshmaker/pull-request/225. Before continuing to update tests, it would be good to decide whether to merge PR #225 .

It looks like #225 is closed now. @cqi, should we proceed with review here?

+1

max_workers is required under python2.

I forgot max_workers is required in python2 before +1.

I forgot max_workers is required in python2 before +1.

Correct myself, I just had a check, that behavior changed in future >= 3.1.1, not python itself, so the simple fix is update requirements.txt to require futures >= 3.1.1 for python2.

Correct myself, I just had a check, that behavior changed in future >= 3.1.1, not python itself, so the simple fix is update requirements.txt to require futures >= 3.1.1 for python2.

I passed max_workers with value 10 that is used in another constructor ThreadPoolExecutor in source code, which probably would be a easy way than upgrading the futures. But, I'm not sure if 10 is a proper value. I didn't pass max_workers because I thought to allow the library to select one automatically according to the number of CPUs in the machine where freshmaker is deployed.

Ready for review.

I passed max_workers with value 10 that is used in another constructor ThreadPoolExecutor in source code, which probably would be a easy way than upgrading the futures. But, I'm not sure if 10 is a proper value. I didn't pass max_workers because I thought to allow the library to select one automatically according to the number of CPUs in the machine where freshmaker is deployed.

How about:

import multiprocessing
max_workers = (multiprocessing.cpu_count() or 1) * 5

which is similar to the implementation in new version of concurrent.futures. And you can switch to use multiprocessing directly since multiprocessing is imported here.

Hm, I would go with 10. We are using threads here to make concurrent connections to PDC. It is not connected to the count of local CPUs, because our threads are not doing any work, they are just waiting for PDC response.

I think we should not query PDC using 10+ connections generally and even 10 threads waiting for PDC response on single CPU are OK.

The way @qwan described is good if the threads are doing some real computing work, but that's not our case.

That makes sense, +1.

Another commit is added for another try to make get_latest_modules easier to understand.

In our case, the latest module in a name:stream is the one with newest version. get_latest_modules has to handle a case after getting modules with the criteria, that is to check if each module with newest version in a name:stream is really a latest module (the top module in the desc order of version), if yes, that module should be rebuilt, otherwise skip it.

The refactor follows this logic. From my point of view, this refactored version should be more straightforward to reflect what problem the code is solving than the previous version.

Please review. If you think it is acceptable, I'll update tests accordingly.

better to tell it's to verify module is the latest one in pdc in comment.

how about rename it to _return_module_if_latest?

@qwan Do you mean add "in PDC" to the docstring?

+1

Commit is amended and rebased on master. Please review again.

+1

\ó/ Congrats! \ó/