iceberg icon indicating copy to clipboard operation
iceberg copied to clipboard
verified publisher icon apache

Core: Add RESTScanReporter to send scan report to REST endpoint

Open nastra opened this issue 3 years ago • 2 comments

depends on https://github.com/apache/iceberg/pull/5427

nastra avatar Aug 01 '22 15:08 nastra

  • Do we need a request object that adds another layer to the JSON? This is minor but it seems odd to have a record that just wraps another record to make the Java types work out (i.e. ScanReportRequest is a RESTRequest).

Not sure if there's an easier way to do it, but essentially our RESTClient requires the body to be a RESTRequest and we can't make ScanReport (part of iceberg-api) just implement RESTRequest (part of iceberg-core) unfortunately.

nastra avatar Sep 05 '22 08:09 nastra

Not sure if there's an easier way to do it, but essentially our RESTClient requires the body to be a RESTRequest and we can't make ScanReport (part of iceberg-api) just implement RESTRequest (part of iceberg-core) unfortunately.

Should we remove the requirement that the object sent is a RESTRequest? That is only a marker interface and I don't think it is particularly useful. If it is causing us to make the REST spec more complicated then it doesn't seem worth the trouble.

rdblue avatar Sep 07 '22 16:09 rdblue

@rdblue thanks for the deep review. I've simplified/improved the things you mentioned and a few other things

nastra avatar Sep 30 '22 11:09 nastra

Nice work, @nastra! This looks ready so I merged it.

rdblue avatar Sep 30 '22 23:09 rdblue