Skip to content

Add goal message to Fate#2492

Merged
milleruntime merged 3 commits intoapache:mainfrom
milleruntime:fate-goal
Feb 14, 2022
Merged

Add goal message to Fate#2492
milleruntime merged 3 commits intoapache:mainfrom
milleruntime:fate-goal

Conversation

@milleruntime
Copy link
Copy Markdown
Contributor

@milleruntime milleruntime commented Feb 14, 2022

@ctubbsii ctubbsii linked an issue Feb 14, 2022 that may be closed by this pull request
@milleruntime
Copy link
Copy Markdown
Contributor Author

Here are some examples of log output:

2022-02-14T10:25:34,906 [fate.Fate] INFO : FATE TxId: 3799578337887111047 Op: TABLE_CREATE Create table ci ONLINE with 19 splits.
2022-02-14T10:37:48,674 [fate.Fate] INFO : FATE TxId: 5505785720937952578 Op: TABLE_COMPACT Compact table (1) with config []
2022-02-14T10:38:18,256 [fate.Fate] INFO : FATE TxId: 2554407174635375739 Op: TABLE_DELETE Delete table ci(1)

@ctubbsii
Copy link
Copy Markdown
Member

@milleruntime I think GitHub is supposed to auto-link PRs to their referenced issues. However, I don't think it works the way you're doing it. It should work when you add the closing keywords to the description or body of a commit message, but it does not seem to work when you add it to the subject line. For consistency (and for less confusion), it's probably best to put these in the body of the commit anyway, so that way when GitHub appends the PR number on merge, it looks a bit nicer:

Add goal message to Fate (#2492)

vs.

Add goal message to Fate. Closes #1249 (#2492)

@EdColeman
Copy link
Copy Markdown
Contributor

From the sample log

2022-02-14T10:37:48,674 [fate.Fate] INFO : FATE TxId: 5505785720937952578 Op: TABLE_COMPACT Compact table (1) with config []
2022-02-14T10:38:18,256 [fate.Fate] INFO : FATE TxId: 2554407174635375739 Op: TABLE_DELETE Delete table ci(1)

What is the number (1) in parentheses? The tableId?

@milleruntime
Copy link
Copy Markdown
Contributor Author

From the sample log

2022-02-14T10:37:48,674 [fate.Fate] INFO : FATE TxId: 5505785720937952578 Op: TABLE_COMPACT Compact table (1) with config []
2022-02-14T10:38:18,256 [fate.Fate] INFO : FATE TxId: 2554407174635375739 Op: TABLE_DELETE Delete table ci(1)

What is the number (1) in parentheses? The tableId?

Yes

@milleruntime milleruntime changed the title Add goal message to Fate. Closes #1249 Add goal message to Fate Feb 14, 2022
milleruntime and others added 2 commits February 14, 2022 12:22
Co-authored-by: Keith Turner <kturner@apache.org>
@milleruntime
Copy link
Copy Markdown
Contributor Author

This method was created because the code used to log fate txids in many different ways (hex and decimal for example)

I forgot about that, thanks @keith-turner. What do you think of changing the transaction ID in the next version to be something more straight forward? Maybe use something like java.util.UUID?

@keith-turner
Copy link
Copy Markdown
Contributor

What is the number (1) in parentheses? The tableId?

In another comment on this thread I suggested a specific way to log FATE IDs that was consistent. The comments about table ids made me think it would be nice to consitently log all ids in the logs. Maybe something like <type>[<id>]. For example FATE[123], TABLE_ID[456], ECID[abd], NAMESPACE_ID[xyz]. I think I will open an issue for this.

What do you think of changing the transaction ID in the next version to be something more straight forward? Maybe use something like java.util.UUID?

@milleruntime I don't have an opinion. What is the motivation for the change?

@milleruntime
Copy link
Copy Markdown
Contributor Author

What do you think of changing the transaction ID in the next version to be something more straight forward? Maybe use something like java.util.UUID?

@milleruntime I don't have an opinion. What is the motivation for the change?

I was just thinking that a stronger type could prevent coding errors like the one I made forgetting to call the method to display the value in HEX.

@keith-turner
Copy link
Copy Markdown
Contributor

I was just thinking that a stronger type could prevent coding errors like the one I made forgetting to call the method to display the value in HEX.

Oh yeah, the code would probably be 10x better if we had a FateTxId type that we used instead of long. Its toString method could consistently format for logging maybe. That could possible encapsulate or extend UUID, but I think we would benefit form a more specific type than UUID or long.

@ctubbsii
Copy link
Copy Markdown
Member

I was just thinking that a stronger type could prevent coding errors like the one I made forgetting to call the method to display the value in HEX.

Oh yeah, the code would probably be 10x better if we had a FateTxId type that we used instead of long. Its toString method could consistently format for logging maybe. That could possible encapsulate or extend UUID, but I think we would benefit form a more specific type than UUID or long.

That seems like a bigger change than updating logging. Should that be considered as part of #2473 ?

@keith-turner
Copy link
Copy Markdown
Contributor

That seems like a bigger change than updating logging. Should that be considered as part of #2473 ?

I was not thinking that change should be made on this PR. Just seems like a good general change to make. I think introducing a fate tx id type to replace long would be a nice change to make by itself for the purposes of review.

@milleruntime milleruntime merged commit aa13bdc into apache:main Feb 14, 2022
@milleruntime milleruntime deleted the fate-goal branch February 14, 2022 19:38
@ctubbsii ctubbsii added this to the 2.1.0 milestone Jul 12, 2024
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.

Allow FATE operations to report goal and status information.

4 participants