[GitHub] drill issue #1021: DRILL-5923: Display name for query state
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1021
Well, I don't have strong preference here, we can use array, as long as
Prasad makes it nicely documented as in your example rather then in one line.
```
String displayNames[] = {
"First Value", // FIRST_VALUE = 0
"Second Value", // SECOND_VALUE = 1
...
};
```
---
[GitHub] drill issue #1021: DRILL-5923: Display name for query state
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/1021
@arina-ielchiieva, it helps to think about the source of the enum. This is
a Protobuf enum. The ordinal values cannot change; they are a contract between
sender and receiver. We can add new ones, or retire old ones, but otherwise the
values are frozen in time.
The array approach captures this reality. We could document the array
better:
```
String displayNames[] = {
"First Value", // FIRST_VALUE = 0
"Second Value", // SECOND_VALUE = 1
...
};
```
We can also do a bounds check:
```
if (enumValue.ordinal() >= displayNames.length) {
return enumValue.toString();
else
return displayNames[enumValue.ordinal());
}
```
But, IMHO a map seems overkill for such a simple task. Yes, it works, but
is unnecessary. As they say, "make it as simple as possible (but no simpler)."
---
[GitHub] drill issue #1021: DRILL-5923: Display name for query state
Github user prasadns14 commented on the issue: https://github.com/apache/drill/pull/1021 @arina-ielchiieva, please review ---
[GitHub] drill issue #1021: DRILL-5923: Display name for query state
Github user arina-ielchiieva commented on the issue: https://github.com/apache/drill/pull/1021 @paul-rogers totally agree with your comments about public API (slipped from my mind when I was writing comment yesterday). I guess then it's fine to go with changes Prasad suggests, I just have left a couple of suggestions to make code clearer. ---
[GitHub] drill issue #1021: DRILL-5923: Display name for query state
Github user paul-rogers commented on the issue:
https://github.com/apache/drill/pull/1021
@arina-ielchiieva, @prasadns14, here is my two cents.
The names and numbers used in the protobuf definitions are part of Drill's
public network API. This API is not versioned, so we can't really change it. If
we changed the names, then, say, C-code or Java code that expects the old names
will break. Being part of the public API, that code may not even be in the
Drill source tree; perhaps someone has generated, say, a Python binding. So,
can't change the public API.
For purely aesthetic reasons, the contributor wishes to change the message
displayed in the UI. This is purely a UI decision (the user is not expected to
map the display names to the Protobuf enums.) And, the display name is subject
to change. Maybe other UIs want to use other names. Maybe we want to show
icons, or abbreviate the names ("Fail", "OK", etc.) And, of course, what if the
display name should have spaces other characters: "In Progress", "In Queue" or
"Didn't Work!". Can't put those in enum names. You get the idea.
For this reason, the mapping from enum values to display names should be
part of the UI, not the network protocol definition. The present change
provides a UI-specific mapping from API Protobuf enum values to display
strings, which seems like a good idea.
So, the key questions are:
* Should we use display strings other than the Protobuf constants (seems a
good idea.)
* Should we do the mapping in Java or in Freemarker? (Java seems simpler.)
Thoughts?
---
[GitHub] drill issue #1021: DRILL-5923: Display name for query state
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/1021
@prasadns14 as far as I understood, you made all these changes to replace
`completed` with `succeeded`. What if you just make changes in State enum
itself, refactor some code and thus no changes in rest part will be required?
From UserBitShared.proto
```
enum QueryState {
STARTING = 0; // query has been scheduled for execution. This is
post-enqueued.
RUNNING = 1;
COMPLETED = 2; // query has completed successfully
CANCELED = 3; // query has been cancelled, and all cleanup is complete
FAILED = 4;
CANCELLATION_REQUESTED = 5; // cancellation has been requested, and
is being processed
ENQUEUED = 6; // query has been enqueued. this is pre-starting.
}
```
After the renaming, please don't forget to regenerate protobuf.
---
[GitHub] drill issue #1021: DRILL-5923: Display name for query state
Github user prasadns14 commented on the issue: https://github.com/apache/drill/pull/1021 @arina-ielchiieva, made changes to avoid display name duplication. Please review ---
