Skip to content

remove s3 bucket polling #1049

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

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

MattShirley
Copy link
Collaborator

@MattShirley MattShirley commented Apr 29, 2025

Added in preparation for front end changes per #1006

@MattShirley MattShirley linked an issue Apr 29, 2025 that may be closed by this pull request
@MattShirley MattShirley requested a review from BenGalewsky April 29, 2025 21:54
@ponyisi
Copy link
Collaborator

ponyisi commented May 1, 2025

Looks good to me. I guess by default the index will be a B-tree and will be useful for the > operator?

@ponyisi
Copy link
Collaborator

ponyisi commented May 1, 2025

Another question, for broader discussion - is it finally time for us to introduce a "capabilities" endpoint? In this case we could use it so the client could verify that this new endpoint is available and switch behavior appropriately. (Mostly I am starting to worry about different sites running different versions of the server software but all users picking up the latest versions of the clients.)

@MattShirley
Copy link
Collaborator Author

Looks good to me. I guess by default the index will be a B-tree and will be useful for the > operator?

Yeah, any index will be B-tree by default and I think it's good practice to over-index anything that might be used to filter values.

@MattShirley
Copy link
Collaborator Author

MattShirley commented May 1, 2025

Another question, for broader discussion - is it finally time for us to introduce a "capabilities" endpoint? In this case we could use it so the client could verify that this new endpoint is available and switch behavior appropriately. (Mostly I am starting to worry about different sites running different versions of the server software but all users picking up the latest versions of the clients.)

Can these capabilities be reduced down to version numbers? If so, you could set the client version as a HTTP header, version the endpoints, and create a middleware to fail any request without a sufficient client version for the endpoint.

I spent some time hacking with Claude and think an interface like this should be possible:

class Endpoint(VersionedResource):
    pass

@Endpoint.require_client(3, 1, 1)
class EndpointV1_0_0(Endpoint):
    def get(self):
        return {"server_version": "v1.0.0"}

@Endpoint.require_client(3, 2, 0)
class EndpointV1_1_0(Endpoint):
    def get(self):
        return {"server_version": "v1.1.0"}
        
api.add_resource(Endpoint, '/my_versioned_endpoint')

@MattShirley
Copy link
Collaborator Author

MattShirley commented May 6, 2025

@ponyisi @BenGalewsky I made major changes to this PR, please take a look. I noticed that files were available in the files table as soon as the request was made (and before results were available) while transformation_results streamed into our database as they became available. So now the endpoint checks transformation_results, which will be polled by the client. Please let me know if this change makes sense and whether we already have an endpoint that offers similar functionality.

@MattShirley MattShirley changed the title add updated since route remove s3 bucket polling May 6, 2025
@MattShirley
Copy link
Collaborator Author

@ponyisi @BenGalewsky added the feedback from Friday, including the cutoff/begin_at value to the endpoint.


args = parser.parse_args()

transform_result_query = TransformationResult.query.filter_by(request_id=request_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have an index on request_id ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do not. I will add one.

@BenGalewsky
Copy link
Contributor

Please contribute to the quality of our codebase with unit tests for this new endpoint

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.

Don't Poll S3 for Bucket Contents
3 participants