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

Merged
merged 31 commits into from
Jun 17, 2025
Merged

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.

@BenGalewsky
Copy link
Contributor

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

@MattShirley
Copy link
Collaborator Author

The feature flag has been added and tests updated/added. This is ready for release from my side.

@MattShirley MattShirley requested a review from ponyisi June 12, 2025 18:02
Copy link
Contributor

@BenGalewsky BenGalewsky left a comment

Choose a reason for hiding this comment

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

Thank you! This is such a great contribution

@ponyisi ponyisi merged commit 4ca0bdd into develop Jun 17, 2025
75 checks passed
@ponyisi ponyisi deleted the 1006-dont-poll-s3-for-bucket-contents branch June 17, 2025 15:39
ponyisi pushed a commit that referenced this pull request Jun 20, 2025
* add backend route plus model, DB migration changes, tests, add capability
ponyisi pushed a commit that referenced this pull request Jun 20, 2025
* add backend route plus model, DB migration changes, tests, add capability
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