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

Add --diff option teraslice-cli #3710

Merged
merged 8 commits into from
Aug 8, 2024
Merged

Conversation

sotojn
Copy link
Contributor

@sotojn sotojn commented Aug 5, 2024

This PR makes the following changes:

  • Adds --diff option to teraslice cli view that will compare the local json job file with what teraslice has on the state cluster
  • Bumps teraslice-cli from v2.1.0 to v2.2.0

Ref to issue #3694

@sotojn sotojn self-assigned this Aug 5, 2024
@sotojn sotojn force-pushed the add-diff-option-teraslice-cli branch from c3e763b to afd09e4 Compare August 5, 2024 20:09
@sotojn sotojn requested review from ciorg and busma13 August 6, 2024 17:17
sotojn and others added 2 commits August 6, 2024 14:35
Co-authored-by: Peter Luitjens <43619525+busma13@users.noreply.github.com>
Copy link
Contributor

@busma13 busma13 left a comment

Choose a reason for hiding this comment

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

Looks good

@sotojn
Copy link
Contributor Author

sotojn commented Aug 6, 2024

Here is an example on how this option looks. In the json file I change the amount of workers from 1 to 2 and run:

earl tjm view test-job.json --diff

Output:

  {
   "_context": "job",
   "_created": "2024-08-01T22:34:50.219Z",
-   "_updated": "2024-08-06T16:03:58.734Z",
+   "_updated": "2024-08-06T16:03:58.777Z",
    "assets": [
      "standard",
      "file"
    ],
    "job_id": "48e59745-ed75-4060-998e-002262abefc3",
    "lifecycle": "once",
    "name": "data-to-s3",
    "operations": [
      {
        "_op": "data_generator",
        "size": 100000
      },
      {
        "_op": "s3_exporter",
        "format": "ldjson",
        "path": "josephorigin"
      }
    ],
-   "workers": 1
+   "workers": 2
  }
--------
job: data-to-s3
id: 48e59745-ed75-4060-998e-002262abefc3
cluster: http://localhost:5678
--------

It does appear that updated seems slightly off all the time and maybe we shouldn't compare it?

@busma13
Copy link
Contributor

busma13 commented Aug 6, 2024

It does appear that updated seems slightly off all the time and maybe we shouldn't compare it?

I think it could be helpful to know that _update will be different between the two records.

@sotojn
Copy link
Contributor Author

sotojn commented Aug 7, 2024

It does appear that updated seems slightly off all the time and maybe we shouldn't compare it?

I think it could be helpful to know that _update will be different between the two records.

Should this be some sort of warning like, "_update field is subject to being slightly different from job file, to whats on the job record in the state cluster". Or should I just ignore this comparison entirely?

@busma13
Copy link
Contributor

busma13 commented Aug 7, 2024

My feeling is that it's fine how it is now and a warning isn't necessary. But maybe it would be better to not compare _update if only the milliseconds are different.

@ciorg
Copy link
Member

ciorg commented Aug 7, 2024

If there is no difference could it out put something like - no difference between the configs instead of printing the whole config, not a big deal it's a pain to do so

Also, what color applies to what config? Green is the local file and red is the cluster config? Maybe something to let people know what applies to what.

So far as the update difference, which for some reason is always different than the cluster value, at least on all the jobs I've looked at so far. ( I'm not sure how the update is calculated, but it seems like that value should be the same for both? Or maybe it shouldn't even have millisecond precision? But that is a different issue)

It is interesting to know when the job was last updated, but if the diff is 100 ms it doesn't really tell me anything useful. So maybe if there's a way to hide that diff unless it's at least a few min difference that would be better.

@sotojn
Copy link
Contributor Author

sotojn commented Aug 8, 2024

I've updated the behavior to include what is being compared to what. I also added logic around the "_updated" field to only display a diff if the time difference is greater than a minute.

Results if "_updated field is less than 1 minute":
Command: earl tjm view data-to-s3.json --diff

  {
    "assets": [
      "standard",
      "file"
    ],
    "job_id": "48e59745-ed75-4060-998e-002262abefc3",
    "lifecycle": "once",
    "name": "data-to-s3",
    "operations": [
      {
        "_op": "data_generator",
        "size": 100000
      },
      {
        "_op": "s3_exporter",
        "format": "ldjson",
        "path": "josephorigin"
      }
    ],
-   "workers": 1   <--- state cluster value
+   "workers": 2   <--- local job file value
  }
--------
job: data-to-s3
id: 48e59745-ed75-4060-998e-002262abefc3
cluster: http://domain123
--------

Results if "_updated field is greater than 1 minute":
Command: earl tjm view data-to-s3.json --diff

  {
-   "_updated": "2024-08-06T16:03:58.734Z",   <--- state cluster value
+   "_updated": "2024-07-06T16:03:58.777Z",   <--- local job file value
    "assets": [
      "standard",
      "file"
    ],
    "job_id": "48e59745-ed75-4060-998e-002262abefc3",
    "lifecycle": "once",
    "name": "data-to-s3",
    "operations": [
      {
        "_op": "data_generator",
        "size": 100000
      },
      {
        "_op": "s3_exporter",
        "format": "ldjson",
        "path": "josephorigin"
      }
    ],
-   "workers": 1   <--- state cluster value
+   "workers": 2   <--- local job file value
  }
--------
job: data-to-s3
id: 48e59745-ed75-4060-998e-002262abefc3
cluster: http://domain123
--------

Copy link
Member

@ciorg ciorg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@busma13 busma13 left a comment

Choose a reason for hiding this comment

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

Looks good.

@busma13 busma13 merged commit 20511ae into master Aug 8, 2024
65 checks passed
@busma13 busma13 deleted the add-diff-option-teraslice-cli branch August 8, 2024 22:14
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.

3 participants