HPCC-31410 DFU superfile copy test #18382

shamser · 2024-03-07T13:12:37Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2024-03-07T13:14:28Z

https://track.hpccsystems.com/browse/HPCC-31410
Jira updated

jakesmith · 2024-03-12T09:57:01Z

testing/regress/ecl/dfucopy.ecl

+
+d1 := DISTRIBUTE(NORMALIZE(v1, 1000, addCount(LEFT, COUNTER)), HASH32(id));
+d2 := DISTRIBUTE(NORMALIZE(v2, 1000, addCount(LEFT, COUNTER)), HASH32(id));
+d3 := DISTRIBUTE(NORMALIZE(v3, 1000, addCount(LEFT, COUNTER)), HASH32(id));


let's simply the test, it doesn't need JOIN's and DISTRIBUTE's (which are well tested elsewhere) in it afaics, to test copy copying a superfile ?
You can use ", DISTRIBUTED" on the inline datasets to ensure they are spread over parts.

jakesmith

@shamser - see comment re. simplifying the test so only/specifically testing a file copy.

Also the JIRA and commit message, don't make it clear this is to adding a test specifically to test super file copies.

jakesmith · 2024-03-12T09:59:36Z

testing/regress/ecl/dfucopy.ecl

+    FileServices.AddSuperFile(prefix + 'superdata',prefix + 'subdata2'),
+    FileServices.AddSuperFile(prefix + 'superdata',prefix + 'subdata3'),
+    FileServices.FinishSuperFileTransaction(),
+    FileServices.Copy(sourceLogicalName := prefix + 'superdata', destinationGroup:= 'mythor', destinationLogicalName := prefix + 'super_copy', ALLOWOVERWRITE := true),


trivial: missing space after 'destinationGroup'

jakesmith · 2024-06-14T16:17:53Z

Not a high priority, but just noticed this seems to have stalled.
@shamser

jakesmith

@shamser - there are some pre-existing copy tests in filecompcopy, but those are specifically for testing copying compressed logical files.

This test is specifically for testing copying a superfile?
If so, I think it would be better to rename to dfusupercopy.ecl for clarity.

Also, can you add a description to the JIRA.

jakesmith · 2024-10-03T12:23:47Z

testing/regress/ecl/dfucopy.ecl

+    FileServices.AddSuperFile(prefix + 'superdata', prefix + 'subdata2'),
+    FileServices.AddSuperFile(prefix + 'superdata', prefix + 'subdata3'),
+    FileServices.FinishSuperFileTransaction(),
+    FileServices.Copy(sourceLogicalName := prefix + 'superdata', destinationGroup := 'mythor', destinationLogicalName := prefix + 'super_copy', ALLOWOVERWRITE := true),


would be more portable if didn't explicitly specify destinationGroup. It will use the source file's by default.

jakesmith · 2024-10-03T12:26:01Z

testing/regress/ecl/dfucopy.ecl

+    FileServices.FinishSuperFileTransaction(),
+    FileServices.Copy(sourceLogicalName := prefix + 'superdata', destinationGroup := 'mythor', destinationLogicalName := prefix + 'super_copy', ALLOWOVERWRITE := true),
+    FileServices.DeleteLogicalFile(prefix + 'super_copy', true),
+    FileServices.DeleteOwnedSubFiles(prefix + 'superdata'),


not really necessary, if you reorder the lines below, and 1st delete the prefix + 'superdata'.

jakesmith · 2024-10-03T12:26:36Z

testing/regress/ecl/dfucopy.ecl

+   STRING20 user;
+END;
+
+layout_names := RECORD


jakesmith · 2024-10-03T12:26:57Z

testing/regress/ecl/dfucopy.ecl

+//noroxie
+//nohthor
+
+import Std.System;


jakesmith · 2024-10-08T10:49:59Z

testing/regress/ecl/dfusupercopy.ecl

+    FileServices.AddSuperFile(prefix + 'superdata', prefix + 'subdata2'),
+    FileServices.AddSuperFile(prefix + 'superdata', prefix + 'subdata3'),
+    FileServices.FinishSuperFileTransaction(),
+    FileServices.Copy(sourceLogicalName := prefix + 'superdata', destinationGroup := Thorlib.group(), destinationLogicalName := prefix + 'super_copy', ALLOWOVERWRITE := true),


destinationGroup := Thorlib.group()

is there any reason to define this at all?
thorlib.group() in k8s doesn't make sense - destinationPlane should be used..

But, neither needs to be defined afaics, defaults would be good.

If the destinationPlane is not defined, an error is reported and it fails to compile. @jakesmith

ok, it happens to work (in k8s) because Thorlib.group() makes no sense in k8s context, so it returns "". That in turn is treated by File.Copy to mean none provided, which then make it default to use the plane/group of the source file being copied.
So passing "" would be more correct here. Can you change to that and check it works?
Assuming it does, it would be sensible to allow this parameter to be optional, omit it, and let it perform the default behaviour if not supplied - which is what we want here.

However, file service calls like this (e.g. Spray too) should really be able to explicitly use a destinationPlane.
I've opened a separate JIRA: https://hpccsystems.atlassian.net/browse/HPCC-32844

jakesmith

@shamser - 1 follow on comment

jakesmith · 2024-10-10T12:34:59Z

@shamser - tagged myself for review - to remind myself to look at this.

jakesmith

@shamser - please see my last comment.

jakesmith

@shamser - looks good. Please squash.

ghalliday · 2024-10-24T13:32:51Z

testing/regress/ecl/dfusupercopy.ecl

+    FileServices.AddSuperFile(prefix + 'superdata', prefix + 'subdata3'),
+    FileServices.FinishSuperFileTransaction(),
+    FileServices.Copy(sourceLogicalName := prefix + 'superdata', destinationGroup := '', destinationLogicalName := prefix + 'super_copy', ALLOWOVERWRITE := true),
+    FileServices.DeleteLogicalFile(prefix + 'super_copy', true),


Should this have code to check that contents of superdata and super_copy match the original?

ghalliday

This has added a check that the source is what we expect, it hasn't checked the target is the same.
When checking the target it should check that the records are in an identical order. (Possibly use COMBINE to efficiently implement that.)

ghalliday

@shamser please squash. This was stuck because it was tagged as a draft request - so it wasn't appearing on any of my lists.

jakesmith · 2025-01-23T16:44:37Z

@shamser - this still needs squashing

Signed-off-by: Shamser Ahmed <[email protected]>

shamser requested a review from jakesmith March 7, 2024 13:13

jakesmith reviewed Mar 12, 2024

View reviewed changes

shamser force-pushed the issue31410 branch from 16250e8 to 0c0b8db Compare October 2, 2024 15:08

shamser changed the base branch from candidate-9.4.x to candidate-9.8.x October 2, 2024 15:10

shamser force-pushed the issue31410 branch 2 times, most recently from f74a525 to 7227950 Compare October 2, 2024 15:12

shamser requested a review from jakesmith October 2, 2024 15:12

jakesmith reviewed Oct 3, 2024

View reviewed changes

shamser force-pushed the issue31410 branch from 7227950 to e9ca1a9 Compare October 4, 2024 10:03

shamser changed the title ~~HPCC-31410 DFU Copy regression test~~ HPCC-31410 DFU superfile copy test Oct 4, 2024

shamser requested a review from jakesmith October 4, 2024 10:07

jakesmith reviewed Oct 8, 2024

View reviewed changes

jakesmith self-requested a review October 10, 2024 12:34

jakesmith reviewed Oct 22, 2024

View reviewed changes

shamser force-pushed the issue31410 branch from e9ca1a9 to f7c74f6 Compare October 22, 2024 09:43

shamser requested a review from jakesmith October 22, 2024 09:43

jakesmith approved these changes Oct 22, 2024

View reviewed changes

shamser force-pushed the issue31410 branch from f7c74f6 to c929db9 Compare October 23, 2024 13:26

ghalliday reviewed Oct 24, 2024

View reviewed changes

shamser requested a review from ghalliday October 28, 2024 12:14

shamser force-pushed the issue31410 branch from c280fde to dc934f9 Compare October 28, 2024 14:46

ghalliday reviewed Nov 1, 2024

View reviewed changes

shamser force-pushed the issue31410 branch from 39079e9 to cbbb2c9 Compare November 15, 2024 15:50

shamser requested a review from ghalliday November 15, 2024 15:50

shamser marked this pull request as draft November 18, 2024 10:36

shamser force-pushed the issue31410 branch from cbbb2c9 to b43a24d Compare November 18, 2024 10:41

ghalliday marked this pull request as ready for review January 9, 2025 17:45

ghalliday approved these changes Jan 9, 2025

View reviewed changes

HPCC-31410 DFU superfile copy test

c1e2b98

Signed-off-by: Shamser Ahmed <[email protected]>

shamser force-pushed the issue31410 branch from b43a24d to c1e2b98 Compare January 24, 2025 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-31410 DFU superfile copy test #18382

HPCC-31410 DFU superfile copy test #18382

shamser commented Mar 7, 2024 •

edited

Loading

github-actions bot commented Mar 7, 2024

jakesmith Mar 12, 2024

jakesmith left a comment

jakesmith Mar 12, 2024

jakesmith commented Jun 14, 2024

jakesmith left a comment

jakesmith Oct 3, 2024

jakesmith Oct 3, 2024

jakesmith Oct 3, 2024

jakesmith Oct 3, 2024

jakesmith Oct 8, 2024

shamser Oct 9, 2024

jakesmith Oct 22, 2024

jakesmith left a comment

jakesmith commented Oct 10, 2024

jakesmith left a comment

jakesmith left a comment

ghalliday Oct 24, 2024

ghalliday left a comment

ghalliday left a comment

jakesmith commented Jan 23, 2025

HPCC-31410 DFU superfile copy test #18382

Are you sure you want to change the base?

HPCC-31410 DFU superfile copy test #18382

Conversation

shamser commented Mar 7, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Mar 7, 2024

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith commented Jun 14, 2024

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith commented Oct 10, 2024

jakesmith left a comment

Choose a reason for hiding this comment

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

jakesmith commented Jan 23, 2025

shamser commented Mar 7, 2024 •

edited

Loading