4
\$\begingroup\$

Boto3 does not seem to implement a generator for RDS instances, instead offering a marker and pagination feature. This code does work for my use case and I have been using it for the entire day without any problems; however, to me, not returning anything in a while True loop seems magically dangerous, it works but leaving it here for general criticism, it is also the first time I implement a generator in python and feel a little insecure about it.

def all_rds_instances(config):
    """
    Gets all the RDS instances in a generator (lazy iterator) so you can implement it as:
    `for instance in all_rds_instances(config):`
    """
    marker = ""
    client = boto3.client('rds', region_name=config["region"])
    pool = []

    # min 20, max 100
    page_size = 20
    while True:
        if len(pool) < 1:
            if marker is None:
                break
            # populate a local pool of instances
            result = client.describe_db_instances(MaxRecords=page_size, Marker=marker)
            marker = result.get("Marker")
            pool = result.get("DBInstances")

        if len(pool) > 0:
            this_instance = pool.pop()
            yield this_instance

A sample implementation follows:

for instance in all_rds_instances({"region": "eu-west-1"}):
    print instance["Endpoint"]["Address"]
\$\endgroup\$
  • \$\begingroup\$ Does order matter? You're using pool.pop(), however I'm wondering if the order if you were to instead use pool.pop(0) would also be acceptable? \$\endgroup\$ – Peilonrayz Jun 15 '17 at 15:16
  • \$\begingroup\$ @Peilonrayz order does not matter, is there a benefit in pop(0) vs pop()? \$\endgroup\$ – Purefan Jun 15 '17 at 15:23
  • 1
    \$\begingroup\$ pop() is preferred performance wise to pop(0). The former being \$O(1)\$, and the latter being \$O(n)\$. I ask as the way I wish to change your code would be cleaner if you could use pop(0), :) \$\endgroup\$ – Peilonrayz Jun 15 '17 at 15:25
5
\$\begingroup\$

Your code is a little hard to read, mostly due to you moving what should be an inner for loop, to be two ifs in an infinite while loop. If you were to instead for loop over pool, then you could make the code easier to understand.

I'd also make page_size be a keyword argument, so the user can change the page size if they wish. And I'd change config to region, as it makes little sense to me, why you're passing a dictionary to extract a single key.

def all_rds_instances(region, page_size=20):
    """
    Gets all the RDS instances in a generator (lazy iterator) so you can implement it as:
    `for instance in all_rds_instances(region):`

    page_size [explain what this does] should be bound between 20 and 100.
    """
    client = boto3.client('rds', region_name=region)
    marker = ""
    pool = []
    while True:
        for instance in pool:
            yield instance

        if marker is None:
            break
        result = client.describe_db_instances(MaxRecords=page_size, Marker=marker)
        marker = result.get("Marker")
        pool = result.get("DBInstances")

This makes reading the code easier, you know that it will exhaust the pool straight away when reading it, and it makes understanding how you infinitely loop a little easier too.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.