Skip to content
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

feat: expose getSchema to TicketResolvers #6218

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

devinrsmith
Copy link
Member

@devinrsmith devinrsmith commented Oct 18, 2024

This exposes the getSchema service to the TicketResolver (motivated by #5339). In addition, this consolidate resolve logic.

In the process of consolidating resolve logic, it was noticed that there was a bug in SharedTicketResolver#flightInfoFor; it is calling FlightExportTicketHelper.descriptorToFlightTicket instead of SharedTicketResolver#descriptorToTicket. Based on the DH client usage which is assuming ticket construction, it is unlikely this affects us in practice since our clients don't use FlightDescriptors. This sort of bug is no longer possible with the resolve logic being consolidated.

Fixes a bug in SharedTicketResolver that was not noticed due to the duplicate logic needed to implement a TicketResolver.
@devinrsmith devinrsmith added this to the Triage milestone Oct 18, 2024
@devinrsmith devinrsmith self-assigned this Oct 18, 2024
@devinrsmith devinrsmith changed the title fix: simplify TicketResolver implementations feat: expose getSchema to TicketResolvers Oct 25, 2024
@devinrsmith devinrsmith modified the milestones: Triage, 0.37.0 Oct 25, 2024
@devinrsmith devinrsmith marked this pull request as ready for review October 25, 2024 13:50
@rcaudy rcaudy requested a review from nbauernfeind October 25, 2024 18:28
devinrsmith added a commit to devinrsmith/deephaven-core that referenced this pull request Oct 25, 2024
*/
public <R> ExportObject<R> map(Function<? super T, ? extends R> f) {
if (session == null) {
final T localResult = result;
Copy link
Member

@nbauernfeind nbauernfeind Oct 27, 2024

Choose a reason for hiding this comment

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

You first need to acquire a ref count on the export object. If you do not, then the export object may be released after reading a non-null result value. If the result is a liveness referent and the export object was the only liveness manager managing the result, then the result will be released if the caller does not own a refcount.

See the comment on ExportObject#get:

        /**
         * WARNING! This method call is only safe to use in the following patterns:
         * <p>
         * 1) If an export (or non-export) {@link ExportBuilder#require}'d this export then the method is valid from
         * within the Callable/Runnable passed to {@link ExportBuilder#submit}.
         * <p>
         * 2) By first obtaining a reference to the {@link ExportObject}, and then observing its state as
         * {@link ExportNotification.State#EXPORTED}. The caller must abide by the Liveness API and dropReference.
         * <p>
         * Example:
         *
         * <pre>
         * {@code
         * <T> T getFromExport(ExportObject<T> export) {
         *     if (export.tryRetainReference()) {
         *         try {
         *             if (export.getState() == ExportNotification.State.EXPORTED) {
         *                 return export.get();
         *             }
         *         } finally {
         *             export.dropReference();
         *         }
         *     }
         *     return null;
         * }
         * }
         * </pre>
         *
         * @return the result of the computed export
         */
        public T get() {

Technically, we should update the comment to highlight the importance of managing the result before the dropReference in the finally block:

if (export.getState() == ExportNotification.State.EXPORTED) {
    final T localResult = export.get();
    if (localResult instanceof LivenessReferent) {
        LivenessScopeStack.peek().manage(localResult);
    }
    return localResult;
}

Note that this mapping implementation could skip managing the result as long as the mapping function doesn't need to own a copy. As in, table operations will manage the result where appropriate, but any function that holds a hard-reference would need to be aware that it must manage the result to ensure it remains valid for the length of time it wants result to be valid.

Whatever approach we choose should be @rcaudy approved.

Do note that session-less non-exports are valid; perhaps we should prefer that approach even if there is some extra threading involved using that code path. I would also be willing to consider a feature that allows for creation of session-less non-exports that are going to be invoked immediately. See #scheduleExport and how it delegates to doExport. This feature would add a third branch that instead calls doExport from within scheduleExport.

Though, if this is not already in an EXPORTED state, then execution must be deferred. So, maybe such a feature has limited use.

* @param logId an end-user friendly identification of the descriptor should an error occur
* @return a FlightInfo describing this flight
*/
SessionState.ExportObject<ByteString> getSchema(@Nullable SessionState session, Flight.FlightDescriptor descriptor,
Copy link
Member

Choose a reason for hiding this comment

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

Can we have this return the pojo.Schema and have the router offer both getSchema and getSchemaBytes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion, more generally that is in line with #6295

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants