move txvalidation flag util to internal/pkg from ledger #927
Conversation
|
Hit Flake: FAB-17611 |
cendhu
force-pushed
the
mv-txvalidation-flag
branch
from
e0de8fb to
6f8aed5
Compare
|
/ci-run |
|
AZP build triggered! |
cendhu
changed the title
cendhu
force-pushed
the
mv-txvalidation-flag
branch
from
6f8aed5 to
fb10fef
Compare
cendhu
changed the title
| @@ -209,7 +209,7 @@ func TestBlockfileMgrGetTxByIdDuplicateTxid(t *testing.T) { | |||
| }, | |||
| []string{"txid-1", "txid-2", "txid-1"}, | |||
| ) | |||
| txValidationFlags := ledgerutil.NewTxValidationFlags(3) | |||
| txValidationFlags := txflags.NewTxValidationFlags(3) | |||
There was a problem hiding this comment.
After moving this here, this functions (and the other one) can be renamed to remove redundant info in the package name and the function name. For instance, this function could simply be renamed as "New".
There was a problem hiding this comment.
yes, I agree. Will do.
There was a problem hiding this comment.
The following changes are done.
TxValidationFlags -> ValidationFlags
NewTxValidationFlags() -> New()
NewTxValidationFlagsSetValue() -> NewWithValues()
There was a problem hiding this comment.
The following changes are done.
TxValidationFlags->ValidationFlags
NewTxValidationFlags()->New()
NewTxValidationFlagsSetValue()->NewWithValues()
Looks good. I guess that "TxValidationFlags" could be changed to "Values" to match others renames. But I am fine merging this as the larger objective is achieved.
There was a problem hiding this comment.
If we do that, wouldn't API become little ugly? -- Values vs NewWithValues()
TxValidationFlags -> Values
NewTxValidationFlags() -> New()
NewTxValidationFlagsSetValue() -> NewWithValues()
The reasoning behind the current naming is that txflags.ValidationFlags denotes that we have txflags and one type of the flag is validation flags. I know that we will not have any more types but this makes code more readable IMO.
There was a problem hiding this comment.
I have contradicted with myself there. With that reasoning, even the New() can be made NewValidationFlags(). Let's see what Gari suggests.
There was a problem hiding this comment.
As I had mentioned earlier, I am fine with any consistent naming. One alternate could be to use txvalidationflags for the package name, so we don't miss validation in the name and at the same time avoid repetition of flags in package name and in the name of functions/types.
There was a problem hiding this comment.
Let's go with what we have now. Don't want to get blocked on this.
The txvalidation util package helps in creating transaction validation flags that would be assigned to the block metadata. This function is used by the v14 and v20 validation as it is needed by these packages to record invalid tx as per policy evaluation. This validation flags is also reused in the mvcc validator to mark valid/invalid transactions. In addition to these two packages, the txflags are referred in so many other packages. Keeping the txflag in core/ledger/util is not correct. Hence, we move the txvalidation util package to the internal/pkgs/txflags. Signed-off-by: senthil <cendhu@gmail.com>
cendhu
force-pushed
the
mv-txvalidation-flag
branch
from
fb10fef to
3ab231a
Compare
cendhu
changed the title
|
@mastersingh24 your approval is still pending. Can you please ack on the following? @manish-sethi and I discussed offline on the package/variable/init function naming. Though it is very minor, we thought of doing it in the correct way as we are already changing 29 files. Though these discussions might look unnecessary, we feel that this knowledge will help in future PRs. Proposed Convention. Current Convention as used in this PR. If you agree with the proposed convention, I will make the modification. |
cendhu
changed the title
Type of change
Description
The txvalidation-flag util package helps in creating transaction validation flags that would be assigned to the block metadata. This function is used by the v14 and v20 validation as it is needed by these packages to record invalid tx as per policy evaluation. This validation flag is also reused in the mvcc validator to mark valid/invalid transactions. Further, the txvalidation-flag util is referred in many other packages including test, hence, we move the util to
internal/pkg/txflagsAdditional details
Before starting the ledger checkpointing code, we would like to refactor the ledger package. If we continue to add new features with the current package structure, after a few years, it will be unmaintainable. We plan to have nearly 10 such PRs. Note that each PR may modify many files and we cannot do much about it. We might be okay to get some review burden than having an unmaintainable code in the longer term.
Related issues
FAB-17677